Eric Blake
2022-Aug-26 12:39 UTC
[Libguestfs] [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
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'?> + > +set -e -u -CMy personal opinion is that 'set -e' is a crutch that should be avoided because of its unintended interaction with functions; 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 EXITIs it intentional that you are not also cleaning up on signals like INT and HUP?> + > +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-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Laszlo Ersek
2022-Aug-30 05:28 UTC
[Libguestfs] [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>
Richard W.M. Jones
2022-Aug-30 13:07 UTC
[Libguestfs] [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
On Fri, Aug 26, 2022 at 07:39:31AM -0500, 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'?Realisticly virt-p2v only makes sense on Linux, both when used to prepare the P2V ISO, as well as the environment in which the ISO boots. eg: Using virt-p2v on (say) FreeBSD to prepare a Linux ISO? The FreeBSD folk would prefer that the built ISO contained FreeBSD, but that would require a small mountain of work.> > + > > +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; 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.I'm confused too: what's the problem with set -e? Compared to _not_ having set -e which allows scripts to continue running after errors. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit