[libvirt] [PATCH] tools: fix variable scope in in check_guests_shutdown

Christian Ehrhardt posted 1 patch 6 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1521459871-18438-1-git-send-email-christian.ehrhardt@canonical.com
Test syntax-check passed
tools/libvirt-guests.sh.in | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[libvirt] [PATCH] tools: fix variable scope in in check_guests_shutdown
Posted by Christian Ehrhardt 6 years ago
libvirt-guests.sh when run with more active guests than requested to
shut down in parallel will run until it times out only shutting down
the first set of guests.

This patch fixes parallel shutdown by fixing a variable scope issue
where check_guests_shutdown unintentionally reset $guests which
prevented further progress.

Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1688508

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@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 d5e68e5..fcada31 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -333,11 +333,11 @@ guest_count()
 check_guests_shutdown()
 {
     uri=$1
-    guests=$2
+    guests_to_check=$2
 
     guests_shutting_down=
     for guest in $guests; do
-        if ! guest_is_on "$uri" "$guest" >/dev/null 2>&1; then
+        if ! guest_is_on "$uri" "$guests_to_check" >/dev/null 2>&1; then
             eval_gettext "Failed to determine state of guest: \$guest. Not tracking it anymore."
             echo
             continue
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tools: fix variable scope in in check_guests_shutdown
Posted by Michal Privoznik 6 years ago
On 03/19/2018 12:44 PM, Christian Ehrhardt wrote:
> libvirt-guests.sh when run with more active guests than requested to
> shut down in parallel will run until it times out only shutting down
> the first set of guests.
> 
> This patch fixes parallel shutdown by fixing a variable scope issue
> where check_guests_shutdown unintentionally reset $guests which
> prevented further progress.
> 
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1688508
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  tools/libvirt-guests.sh.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Oops, yes.

ACKed and pushed.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tools: fix variable scope in in check_guests_shutdown
Posted by Martin Kletzander 6 years ago
On Mon, Mar 19, 2018 at 12:44:31PM +0100, Christian Ehrhardt wrote:
>libvirt-guests.sh when run with more active guests than requested to
>shut down in parallel will run until it times out only shutting down
>the first set of guests.
>
>This patch fixes parallel shutdown by fixing a variable scope issue
>where check_guests_shutdown unintentionally reset $guests which
>prevented further progress.
>
>Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1688508
>
>Signed-off-by: Christian Ehrhardt <christian.ehrhardt@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 d5e68e5..fcada31 100644
>--- a/tools/libvirt-guests.sh.in
>+++ b/tools/libvirt-guests.sh.in
>@@ -333,11 +333,11 @@ guest_count()
> check_guests_shutdown()
> {
>     uri=$1
>-    guests=$2
>+    guests_to_check=$2
>
>     guests_shutting_down=
>     for guest in $guests; do
>-        if ! guest_is_on "$uri" "$guest" >/dev/null 2>&1; then
>+        if ! guest_is_on "$uri" "$guests_to_check" >/dev/null 2>&1; then
>             eval_gettext "Failed to determine state of guest: \$guest. Not tracking it anymore."

So I wanted to also send a patch for this                        ^^^^^^
to be adjusted and realized it's not needed as that's a different
variable.

However, I also realized we're not using 'local' anywhere.  We should
probably do that (maybe instead of renames in the future), so I'll add
it to the BiteSizedTasks, I got to the half of the file and found out it
can't be done with a regexp replace =)

>             echo
>             continue
>-- 
>2.7.4
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tools: fix variable scope in in check_guests_shutdown
Posted by Christian Ehrhardt 6 years ago
On Thu, Mar 22, 2018 at 5:26 PM, Martin Kletzander <mkletzan@redhat.com>
wrote:

> On Mon, Mar 19, 2018 at 12:44:31PM +0100, Christian Ehrhardt wrote:
>
>> libvirt-guests.sh when run with more active guests than requested to
>> shut down in parallel will run until it times out only shutting down
>> the first set of guests.
>>
>> This patch fixes parallel shutdown by fixing a variable scope issue
>> where check_guests_shutdown unintentionally reset $guests which
>> prevented further progress.
>>
>> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1688508
>>
>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@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 d5e68e5..fcada31 100644
>> --- a/tools/libvirt-guests.sh.in
>> +++ b/tools/libvirt-guests.sh.in
>> @@ -333,11 +333,11 @@ guest_count()
>> check_guests_shutdown()
>> {
>>     uri=$1
>> -    guests=$2
>> +    guests_to_check=$2
>>
>>     guests_shutting_down=
>>     for guest in $guests; do
>> -        if ! guest_is_on "$uri" "$guest" >/dev/null 2>&1; then
>> +        if ! guest_is_on "$uri" "$guests_to_check" >/dev/null 2>&1; then
>>             eval_gettext "Failed to determine state of guest: \$guest.
>> Not tracking it anymore."
>>
>
> So I wanted to also send a patch for this                        ^^^^^^
> to be adjusted and realized it's not needed as that's a different
> variable.
>
> However, I also realized we're not using 'local' anywhere.  We should
> probably do that (maybe instead of renames in the future), so I'll add
> it to the BiteSizedTasks, I got to the half of the file and found out it
> can't be done with a regexp replace =)
>

Hi Martin,
I agree that a general overhaul would be nice, but at least I couldn't
affort the time this week to do so (away next week).
I guess you hit the fact that some functions intentionally do so with your
regexp :-)
Also I was unsure about different shell implementations in doing some of
the changes, so I didn't want to do a bigger change in a rush.

Thanks for adding it to the BiteSizedTasks - I think that is just the right
thing to do.


>             echo
>>             continue
>> --
>> 2.7.4
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list