[libvirt] libvirt-guest.sh bug fixes
Two small changes, before I forget about submitting them... First one affects all environments the same. The list of UIDs which is generated has each element on a separate line. And using quotes in the echo preserves those newlines. However the processing assumes one line per URI and all UIDs separated by spaces. So without dropping the quotes only one guest will get shutdown/suspended. The second change is for Xen environments only. Domain-0 appears in the list of guests and it is a persistent one. So on shutdown, the script tries to stop Domain-0 (which is not working) and then waits the whole timeout for it to stop. -Stefan
The list file expects all guest UUIDs on the same line as the URI which the guests run on. This does not happen when the list is echo'ed in quotes. When stripping the quotes, newlines get transformed into spaces. Without this, only the first guest on the list is actually handled. Based on a fix by Omar Siam <simar@gmx.net> Bug-Ubuntu: http://bugs.launchpad.net/bugs/1591695 Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- tools/libvirt-guests.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index 7f74b85..7380b4b 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -499,7 +499,7 @@ stop() { fi if [ -n "$list" ]; then - echo "$uri" "$list" >>"$LISTFILE" + echo "$uri" $list >>"$LISTFILE" fi done set +f -- 1.9.1
On 10/07/2016 02:56 AM, Stefan Bader wrote:
The list file expects all guest UUIDs on the same line as the URI which the guests run on. This does not happen when the list is echo'ed in quotes. When stripping the quotes, newlines get transformed into spaces. Without this, only the first guest on the list is actually handled.
Based on a fix by Omar Siam <simar@gmx.net>
Bug-Ubuntu: http://bugs.launchpad.net/bugs/1591695
Signed-off-by: Stefan Bader <stefan.bader@canonical.com> --- tools/libvirt-guests.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index 7f74b85..7380b4b 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -499,7 +499,7 @@ stop() { fi
if [ -n "$list" ]; then - echo "$uri" "$list" >>"$LISTFILE" + echo "$uri" $list >>"$LISTFILE"
This is one case where "" is harmful - we WANT the shell to perform word-splitting, and the content of $list should be safe for splitting (ie. since all elements of the list are UUIDs, none of them should contain any other whitespace or shell metacharacters that would misbehave by losing the ""). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
With newer versions of libvirt Domain-0 is again visible in the list of running guests but it should not be considered as a guest for shutdown or suspend. Signed-off-by Stefan Bader <stefan.bader@canonical.com> --- tools/libvirt-guests.sh.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index 7380b4b..791d927 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -121,7 +121,7 @@ list_guests() { return 1 fi - echo $list + echo "$list" | grep -v 00000000-0000-0000-0000-000000000000 } # guest_name URI UUID @@ -539,7 +539,7 @@ gueststatus() { for uri in $URIS; do set +f echo "* $uri URI:" - retval run_virsh "$uri" list || echo + retval run_virsh "$uri" list | grep -v "Domain-0" || echo done set +f } -- 1.9.1
On 10/07/2016 02:56 AM, Stefan Bader wrote:
With newer versions of libvirt Domain-0 is again visible in the list of running guests but it should not be considered as a guest for shutdown or suspend.
Signed-off-by Stefan Bader <stefan.bader@canonical.com> --- tools/libvirt-guests.sh.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK.
diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index 7380b4b..791d927 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -121,7 +121,7 @@ list_guests() { return 1 fi
- echo $list + echo "$list" | grep -v 00000000-0000-0000-0000-000000000000 }
# guest_name URI UUID @@ -539,7 +539,7 @@ gueststatus() { for uri in $URIS; do set +f echo "* $uri URI:" - retval run_virsh "$uri" list || echo + retval run_virsh "$uri" list | grep -v "Domain-0" || echo done set +f }
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07.10.2016 15:56, Stefan Bader wrote:
Two small changes, before I forget about submitting them...
First one affects all environments the same. The list of UIDs which is generated has each element on a separate line. And using quotes in the echo preserves those newlines. However the processing assumes one line per URI and all UIDs separated by spaces. So without dropping the quotes only one guest will get shutdown/suspended.
The second change is for Xen environments only. Domain-0 appears in the list of guests and it is a persistent one. So on shutdown, the script tries to stop Domain-0 (which is not working) and then waits the whole timeout for it to stop.
-Stefan
Eric ACKed both of them and I've just pushed them. Michal
On 10/07/2016 03:56 AM, Stefan Bader wrote:
Two small changes, before I forget about submitting them...
First one affects all environments the same. The list of UIDs which is generated has each element on a separate line. And using quotes in the echo preserves those newlines. However the processing assumes one line per URI and all UIDs separated by spaces. So without dropping the quotes only one guest will get shutdown/suspended.
The second change is for Xen environments only. Domain-0 appears in the list of guests and it is a persistent one. So on shutdown, the script tries to stop Domain-0 (which is not working) and then waits the whole timeout for it to stop.
Note these patches were flagged as problematic a few months back: https://bugzilla.redhat.com/show_bug.cgi?id=1335585 Stefan, have you heard about that issue? Was it resolved with these versions? Thanks, Cole
On 10.10.2016 17:06, Cole Robinson wrote:
On 10/07/2016 03:56 AM, Stefan Bader wrote:
Two small changes, before I forget about submitting them...
First one affects all environments the same. The list of UIDs which is generated has each element on a separate line. And using quotes in the echo preserves those newlines. However the processing assumes one line per URI and all UIDs separated by spaces. So without dropping the quotes only one guest will get shutdown/suspended.
The second change is for Xen environments only. Domain-0 appears in the list of guests and it is a persistent one. So on shutdown, the script tries to stop Domain-0 (which is not working) and then waits the whole timeout for it to stop.
Note these patches were flagged as problematic a few months back: https://bugzilla.redhat.com/show_bug.cgi?id=1335585
Stefan, have you heard about that issue? Was it resolved with these versions?
I did not hear about that before. But revisiting things again I think what happened is that the Xen patch which I had done before (but at that time forgot to submit upstream) was adding quotes there because without, the grep does not work. Back then I did not realize this broke the shutdown as that also had quotes around the list. All the other users of the list are ok because they wrap the processing into for ... loops and for those newline or space does not matter. So together with the other patch wich drops the quotes when writing the list file this should be resolved. list_guests returns a list of uids (separated by newline), the output of the info message is correctly showing names in one line separated by komma, and the produced list file had only one line per uri with uids separated by spaces. -Stefan
Thanks, Cole
On 10.10.2016 18:32, Stefan Bader wrote:
On 10.10.2016 17:06, Cole Robinson wrote:
On 10/07/2016 03:56 AM, Stefan Bader wrote:
Two small changes, before I forget about submitting them...
First one affects all environments the same. The list of UIDs which is generated has each element on a separate line. And using quotes in the echo preserves those newlines. However the processing assumes one line per URI and all UIDs separated by spaces. So without dropping the quotes only one guest will get shutdown/suspended.
The second change is for Xen environments only. Domain-0 appears in the list of guests and it is a persistent one. So on shutdown, the script tries to stop Domain-0 (which is not working) and then waits the whole timeout for it to stop.
Note these patches were flagged as problematic a few months back: https://bugzilla.redhat.com/show_bug.cgi?id=1335585
Stefan, have you heard about that issue? Was it resolved with these versions?
I did not hear about that before. But revisiting things again I think what happened is that the Xen patch which I had done before (but at that time forgot to submit upstream) was adding quotes there because without, the grep does not work. Back then I did not realize this broke the shutdown as that also had quotes around the list. All the other users of the list are ok because they wrap the processing into for ... loops and for those newline or space does not matter.
I think the surprising part is that the calls to virsh like: list=$(virsh ... list) seem to retain the newline seperator despite having no quotes. Only when using echo with the unquoted variable seems to do that. That is with dash as sh at least.
So together with the other patch wich drops the quotes when writing the list file this should be resolved. list_guests returns a list of uids (separated by newline), the output of the info message is correctly showing names in one line separated by komma, and the produced list file had only one line per uri with uids separated by spaces.
-Stefan
Thanks, Cole
On 10/10/2016 11:48 AM, Stefan Bader wrote:
I did not hear about that before. But revisiting things again I think what happened is that the Xen patch which I had done before (but at that time forgot to submit upstream) was adding quotes there because without, the grep does not work. Back then I did not realize this broke the shutdown as that also had quotes around the list. All the other users of the list are ok because they wrap the processing into for ... loops and for those newline or space does not matter.
I think the surprising part is that the calls to virsh like:
list=$(virsh ... list)
seem to retain the newline seperator despite having no quotes. Only when using echo with the unquoted variable seems to do that. That is with dash as sh at least.
That's true for ALL shells. Word splitting does NOT happen during variable assignment. list=$() is _strictly_ equivalent to list="$()", no matter the shell. It's just that there are so many other places where $() and "$()" behave differently that it's (usually) a good habit to always use "", rather than learning the rules on when you NEED them, vs. telling the special cases when you DON'T want them apart from the cases where they are optional. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10.10.2016 20:43, Eric Blake wrote:
On 10/10/2016 11:48 AM, Stefan Bader wrote:
I did not hear about that before. But revisiting things again I think what happened is that the Xen patch which I had done before (but at that time forgot to submit upstream) was adding quotes there because without, the grep does not work. Back then I did not realize this broke the shutdown as that also had quotes around the list. All the other users of the list are ok because they wrap the processing into for ... loops and for those newline or space does not matter.
I think the surprising part is that the calls to virsh like:
list=$(virsh ... list)
seem to retain the newline seperator despite having no quotes. Only when using echo with the unquoted variable seems to do that. That is with dash as sh at least.
That's true for ALL shells. Word splitting does NOT happen during variable assignment. list=$() is _strictly_ equivalent to list="$()", no matter the shell.
Funny that with all those years working with shells I never memorized that as a fact. :) Guess it was enough to adhere to the good habit and just use "$()" all the time. So just as a wrap up, the first patch ends up not being a fix in the strict sense as before the second patch word splitting would happen in the list_guest function. Luckily it neither breaks anything on its own as the result is the same whether list was already split or not. Somehow I subconsciously reordered it to come before the dom0 patch which I had done here before (without noticing that the list file is then broken for more than one guest to shutdown). Somehow the big part of adding the grep in the dom0 patch made me miss the additional quotes around the variable. In hindsight maybe the better way would have been to add a new line which filters the dom0 uid and reassigns the result to list and keep the echo line unmodified. But then, the end result now should be working and robust enough... -Stefan
It's just that there are so many other places where $() and "$()" behave differently that it's (usually) a good habit to always use "", rather than learning the rules on when you NEED them, vs. telling the special cases when you DON'T want them apart from the cases where they are optional.
On 10/10/2016 12:32 PM, Stefan Bader wrote:
On 10.10.2016 17:06, Cole Robinson wrote:
On 10/07/2016 03:56 AM, Stefan Bader wrote:
Two small changes, before I forget about submitting them...
First one affects all environments the same. The list of UIDs which is generated has each element on a separate line. And using quotes in the echo preserves those newlines. However the processing assumes one line per URI and all UIDs separated by spaces. So without dropping the quotes only one guest will get shutdown/suspended.
The second change is for Xen environments only. Domain-0 appears in the list of guests and it is a persistent one. So on shutdown, the script tries to stop Domain-0 (which is not working) and then waits the whole timeout for it to stop.
Note these patches were flagged as problematic a few months back: https://bugzilla.redhat.com/show_bug.cgi?id=1335585
Stefan, have you heard about that issue? Was it resolved with these versions?
I did not hear about that before. But revisiting things again I think what happened is that the Xen patch which I had done before (but at that time forgot to submit upstream) was adding quotes there because without, the grep does not work. Back then I did not realize this broke the shutdown as that also had quotes around the list. All the other users of the list are ok because they wrap the processing into for ... loops and for those newline or space does not matter.
So together with the other patch wich drops the quotes when writing the list file this should be resolved. list_guests returns a list of uids (separated by newline), the output of the info message is correctly showing names in one line separated by komma, and the produced list file had only one line per uri with uids separated by spaces.
Sounds good, I'll close that bug then Thanks, Cole
participants (4)
-
Cole Robinson -
Eric Blake -
Michal Privoznik -
Stefan Bader