Libguestfs - Aug 2022 - [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"

On 08/26/22 14:39, Eric Blake wrote:
> On Fri, Aug 26, 2022 at 01:39:05PM +0200, Laszlo Ersek wrote:
>> Extract and somewhat generalize the recipe for the $(PHYSICAL_MACHINE)
>> target to a separate shell script. In preparation for the multiple
steps
>> we're going to introduce later, redirect virt-builder to a temp
file at
>> first (placed in the same directory as the finally expected disk
image),
>> and rename that file upon success.
>>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
> 
>> +++ b/make-physical-machine.sh
>> @@ -0,0 +1,37 @@
>> +#!/bin/bash -
> 
> See the response in the other thread about not needing the - here.
> Are we sure that /bin/bash is on all systems where this script will be
> run, or is it better as '#!/bin/env bash'?
I had grepped the virt-p2v and virt-v2v shell scripts for shebangs:

      1 #!/bin/sh
      2 #!/bin/bash
      4 #!/usr/bin/env perl
     17 #!/bin/bash -

      1 #!/bin/bash
      1 #!/usr/bin/env python3
      1 #!/usr/bin/sh
      5 #!/usr/bin/env perl
     67 #!/bin/bash -

The thinking seems to have been that
- python3 and perl may "move",
- bash is considered always available.

So I wouldn't break consistency with "#!/bin/bash". I did feel
like
breaking consistency with the hyphen "#!/bin/bash -" -- in that,
minimally, I would write "#!/bin/bash --".

Anyway, as a temporary approach, I just stuck with the tradition (the
hyphen), and ended up posting the patch like that. It's not
"wrong"
technically, just strange to my taste. I no longer feel strongly about
it (but I'm happy to remove the hyphen if that's desired).
> 
>> +
>> +set -e -u -C
> 
> My personal opinion is that 'set -e' is a crutch that should be
> avoided because of its unintended interaction with functions;
Can you please elaborate?

POSIX writes, "When a function is executed, it shall have the
syntax-error and variable-assignment properties described for special
built-in utilities in the enumerated list at the beginning of Special
Built-In Utilities", and I've checked that list -- I don't know
what you
mean.
> but I'm
> not adamant enough about it to tell people to rip it out of scripts.
> For short scripts, like this one, it's easy enough to check that we
> aren't tripping over any of -e's surprises.
> 
>> +
>> +disk>> +
>> +cleanup()
>> +{
>> +  set +e
>> +  if test -n "$disk"; then
>> +    rm -f -- "$disk"
>> +    disk>> +  fi
>> +}
>> +
>> +trap cleanup EXIT
> 
> Is it intentional that you are not also cleaning up on signals like
> INT and HUP?
Yes, as EXIT covers those.

Easy to test with:

------
#!/bin/bash
cleanup()
{
  echo done >zzz
}
trap cleanup EXIT
sleep 100
------

rm -f zzz
./hello.sh

and then hitting either ^C or Alt-F4 in/on the terminal window.

It's for a similar reason I didn't specify ERR -- ERR applies roughly
under the same conditions where "set -e" causes the shell to exit, so
"set -e" causes (in effect) the EXIT trap handler to cover ERR too.

Laszlo
> 
>> +
>> +output=$1
>> +outdir=$(dirname -- "$output")
>> +disk=$(mktemp -p "$outdir" physical-machine.tmp.XXXXXXXXXX)
>> +virt-builder --format raw -o "$disk" fedora-35
>> +mv -- "$disk" "$output"
>> +disk>

On Tue, Aug 30, 2022 at 07:28:43AM +0200, Laszlo Ersek
wrote:
> I had grepped the virt-p2v and virt-v2v shell scripts for shebangs:
> 
>       1 #!/bin/sh
>       2 #!/bin/bash
>       4 #!/usr/bin/env perl
>      17 #!/bin/bash -
> 
>       1 #!/bin/bash
>       1 #!/usr/bin/env python3
>       1 #!/usr/bin/sh
>       5 #!/usr/bin/env perl
>      67 #!/bin/bash -
> 
> The thinking seems to have been that
> - python3 and perl may "move",
> - bash is considered always available.
For p2v, yeah, we are pretty much guaranteed to be running on Linux,
and therefore have bash (even if /bin/sh is not bash).
> > 
> >> +
> >> +set -e -u -C
> > 
> > My personal opinion is that 'set -e' is a crutch that should
be
> > avoided because of its unintended interaction with functions;
> 
> Can you please elaborate?
> 
> POSIX writes, "When a function is executed, it shall have the
> syntax-error and variable-assignment properties described for special
> built-in utilities in the enumerated list at the beginning of Special
> Built-In Utilities", and I've checked that list -- I don't
know what you
> mean.
Here's a good writeup:
http://mywiki.wooledge.org/BashFAQ/105

One of the simplest demonstrations on that page:

$ bash -c 'set -e; f() { set -e; false; echo huh; }; f; echo survived'
$ bash -c 'set -e; f() { set -e; false; echo huh; }; f && echo
survived'
huh
survived

That is, using 'set -e' to end function f early "works", but
ONLY in
situations where f itself is not invoked in a context where 'set -e'
is suppressed because of a conditional in an outer scope.  And once
'if' or '&&' suppresses 'set -e', you cannot
re-enable it within f
itself.  Context-sensitive behavior of your function body based on the
context of the caller is NOT intuitive.

That said, if you KNOW that -e behaves non-intuitively, and plan to
write your script with that in mind, it can be helpful.  The complaint
is that because -e is disabled in so many situations, it is harder to
prove that -e catches all the scenarios that you WANTED to be caught
than it is to just write the error handling yourself.
> 
> > but I'm
> > not adamant enough about it to tell people to rip it out of scripts.
> > For short scripts, like this one, it's easy enough to check that
we
> > aren't tripping over any of -e's surprises.
> > 
> >> +
> >> +disk> >> +
> >> +cleanup()
> >> +{
> >> +  set +e
> >> +  if test -n "$disk"; then
> >> +    rm -f -- "$disk"
> >> +    disk> >> +  fi
> >> +}
> >> +
> >> +trap cleanup EXIT
> > 
> > Is it intentional that you are not also cleaning up on signals like
> > INT and HUP?
> 
> Yes, as EXIT covers those.
Okay; currently true enough for bash, but not portable to other
shells.  In fact, the Austin Group visited that topic just this week:

https://austingroupbugs.net/view.php?id=621
 After 2018 edition page 2420 line 77499 section 2.14 trap, add a new
paragraph:
    The EXIT condition shall occur when the shell terminates normally
    (exits), and may occur when the shell terminates abnormally as a
    result of delivery of a signal (other than SIGKILL) whose trap
    action is the default.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org