[PATCH] tools: fix libvirt-guests.sh text assignments

[Christian Ehrhardt]

On Thu, Aug 20, 2020 at 10:50 AM Michael Chapman <mike at very.puzzling.org> wrote:
>
> On Thu, 20 Aug 2020, Christian Ehrhardt wrote:
> > On Wed, Aug 19, 2020 at 12:15 PM Christian Ehrhardt
> > <christian.ehrhardt at canonical.com> wrote:
> > >
> > > In libvirt 6.6 stopping guests with libvirt-guests.sh is broken.
> > > As soon as there is more than one guest one can see
> > > `systemctl stop libvirt-guests` faiing and in the log we see:
> > >   libvirt-guests.sh[2455]: Running guests on default URI:
> > >   libvirt-guests.sh[2457]: /usr/lib/libvirt/libvirt-guests.sh: 120:
> > >       local: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae2: bad variable name
> > >   libvirt-guests.sh[2462]: no running guests.
> > >
> > > That is due do mutliple guests becoming a list of UUIDs. Without
> > > recognizing this as one single string the assignment breaks when using 'local'
> > > (which was recently added in 6.3.0). This is because local is defined as
> > >   local [option] [name[=value] ... | - ]
> > > which makes the shell trying handle the further part of the string as
> > > variable names. In the error above that string isn't a valid variable
> > > name triggering the issue that is seen.
> > >
> > > To resolve that 'textify' all assignments that are strings or potentially
> > > can become such lists (even if they are not using the local qualifier).
> >
> > Just to illustrate the problem, this never was great and could cause
> > warnings/errors,
> > but amplified due to the 'local' it makes the script break now.
>
> Arguably the big problem here is that 'local' isn't actually specified by
> POSIX, so can not be used in a portable /bin/sh script. (It might end up
> in POSIX eventually, see [1].)
>
> If this were a Bash script, then all of those variable assignments
> (whether they're local or not) would work as expected:
>
>     $ echo $BASH_VERSION
>     5.0.17(1)-release
>     $ uuid='2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3'
>     $ foo() { x=$uuid; echo "<$x>"; }
>     $ bar() { local x=$uuid; echo "<$x>"; }
>     $ foo
>     <2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3>
>     $ bar
>     <2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3>

Thanks for these initial tests showing that this is a real issue
(depending on shell&mode being used).

> This works even when Bash is running in POSIX mode.

Dash in POSIX mode (used as /bin/sh interpreter) was what I used to
spot the issue.
Maybe because bash in POSIX mode works is why it wasn't spotted before.


> But POSIX shells that do not support 'local' seem to be rare

Agreed

> , and your
> suggested change would make these assignments work even on shells that do
> not apply special parsing rules for it.

Thanks, I also have pushed this through a bunch of tests that use
libvirt-guests.sh on multiple situations and architectures.
They all worked fine with the fix (and were the ones spotting the
issue in the first place).
So after adding these extra bits of info to the commit message, fixing
a typo in there and running a gitlab CI pipeline on it I'm pushing the
commit.

Thanks Michael for the extra thoughts on this!

>
> [1] https://www.austingroupbugs.net/bug_view_page.php?bug_id=767



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd