When the job monitoring logic was refactored, these two commands
were not converted properly and the result is that a successful
dump or migration (char '0') would be reported as a failed one
(int 48) instead.

Fixes: dc0771cfa2e78ffecd7c8234538ee548748d7bef
Reported-by: Brian Rak <brak@gameservers.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 tools/virsh-domain.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index d52eb7bc2f..502685f44b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5436,7 +5436,6 @@ static const vshCmdOptDef opts_dump[] = {
 static void
 doDump(void *opaque)
 {
-    char ret = '1';
     virshCtrlData *data = opaque;
     vshControl *ctl = data->ctl;
     const vshCmd *cmd = data->cmd;
@@ -5508,7 +5507,7 @@ doDump(void *opaque)
         }
     }
 
-    ret = '0';
+    data->ret = 0;
  out:
 #ifndef WIN32
     pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL);
@@ -5516,7 +5515,6 @@ doDump(void *opaque)
 #endif /* !WIN32 */
     if (dom)
         virshDomainFree(dom);
-    data->ret = ret;
     g_main_loop_quit(data->eventLoop);
 }
 
@@ -10722,7 +10720,6 @@ static const vshCmdOptDef opts_migrate[] = {
 static void
 doMigrate(void *opaque)
 {
-    char ret = '1';
     virDomainPtr dom = NULL;
     const char *desturi = NULL;
     const char *opt = NULL;
@@ -11001,14 +10998,14 @@ doMigrate(void *opaque)
 
     if (flags & VIR_MIGRATE_PEER2PEER || vshCommandOptBool(cmd, "direct")) {
         if (virDomainMigrateToURI3(dom, desturi, params, nparams, flags) == 0)
-            ret = '0';
+            data->ret = 0;
     } else {
         /* For traditional live migration, connect to the destination host directly. */
         virDomainPtr ddom = NULL;
 
         if ((ddom = virDomainMigrate3(dom, dconn, params, nparams, flags))) {
             virshDomainFree(ddom);
-            ret = '0';
+            data->ret = 0;
         }
     }
 
@@ -11019,7 +11016,6 @@ doMigrate(void *opaque)
 #endif /* !WIN32 */
     virTypedParamsFree(params, nparams);
     virshDomainFree(dom);
-    data->ret = ret;
     g_main_loop_quit(data->eventLoop);
     return;
 
-- 
2.25.3

On Tue, Apr 21, 2020 at 07:15:00PM +0200, Andrea Bolognani wrote:
> When the job monitoring logic was refactored, these two commands
> were not converted properly and the result is that a successful
> dump or migration (char '0') would be reported as a failed one
> (int 48) instead.
> 
> Fixes: dc0771cfa2e78ffecd7c8234538ee548748d7bef
> Reported-by: Brian Rak <brak@gameservers.com>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  tools/virsh-domain.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, 2020-04-21 at 18:25 +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 21, 2020 at 07:15:00PM +0200, Andrea Bolognani wrote:
> > When the job monitoring logic was refactored, these two commands
> > were not converted properly and the result is that a successful
> > dump or migration (char '0') would be reported as a failed one
> > (int 48) instead.
> > 
> > Fixes: dc0771cfa2e78ffecd7c8234538ee548748d7bef
> > Reported-by: Brian Rak <brak@gameservers.com>
> > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> > ---
> >  tools/virsh-domain.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks, pushed now. And thanks again to Brian for reporting the issue
and tracking it down!

Do you think we should backport this to the 6.1 and 6.2 mainteinance
branches? It fixes a fairly bad regression... But I'm not very
familiar with how we handle those branches.

-- 
Andrea Bolognani / Red Hat / Virtualization

On a Wednesday in 2020, Andrea Bolognani wrote:
>On Tue, 2020-04-21 at 18:25 +0100, Daniel P. Berrangé wrote:
>> On Tue, Apr 21, 2020 at 07:15:00PM +0200, Andrea Bolognani wrote:
>> > When the job monitoring logic was refactored, these two commands
>> > were not converted properly and the result is that a successful
>> > dump or migration (char '0') would be reported as a failed one
>> > (int 48) instead.
>> >
>> > Fixes: dc0771cfa2e78ffecd7c8234538ee548748d7bef
>> > Reported-by: Brian Rak <brak@gameservers.com>
>> > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>> > ---
>> >  tools/virsh-domain.c | 10 +++-------
>> >  1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>Thanks, pushed now. And thanks again to Brian for reporting the issue
>and tracking it down!
>
>Do you think we should backport this to the 6.1 and 6.2 mainteinance
>branches? It fixes a fairly bad regression... But I'm not very
>familiar with how we handle those branches.
>

We don't because nobody was using them anymore.

Jano

>-- 
>Andrea Bolognani / Red Hat / Virtualization
>