[PATCH] virsh: Fix return code for dump

Xuyandong (Yandong Xu) posted 1 patch 3 years, 10 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/7CECC2DFC21538489F72729DF5EFB4D90FECA8B4@DGGEMM501-MBX.china.huawei.com
tools/virsh-domain.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
[PATCH] virsh: Fix return code for dump
Posted by Xuyandong (Yandong Xu) 3 years, 10 months ago
>From 4899d04fab878d5a0e13f01b8b137af233bc7d33 Mon Sep 17 00:00:00 2001
From: Xu Yandong <xuyandong2@huawei.com>
Date: Mon, 4 May 2020 16:36:19 +0800
Subject: [PATCH] virsh: Fix return code for dump

After the commit dc0771c, ret variable no longer
represents the status of the return code, use
data->ret replace it.

Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
---
 tools/virsh-domain.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0704529458..61cd72f714 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5522,7 +5522,6 @@ static bool
 cmdDump(vshControl *ctl, const vshCmd *cmd)
 {
     virDomainPtr dom;
-    bool ret = false;
     bool verbose = false;
     const char *name = NULL;
     const char *to = NULL;
@@ -5556,12 +5555,12 @@ cmdDump(vshControl *ctl, const vshCmd *cmd)
 
     virThreadJoin(&workerThread);
 
-    if (!ret)
+    if (!data.ret)
         vshPrintExtra(ctl, _("\nDomain %s dumped to %s\n"), name, to);
 
  cleanup:
     virshDomainFree(dom);
-    return !ret;
+    return !data.ret;
 }
 
 static const vshCmdInfo info_screenshot[] = {
-- 
2.21.0


Re: [PATCH] virsh: Fix return code for dump
Posted by Andrea Bolognani 3 years, 10 months ago
On Mon, 2020-05-04 at 09:48 +0000, Xuyandong (Yandong Xu) wrote:
> +++ b/tools/virsh-domain.c
> @@ -5522,7 +5522,6 @@ static bool
>  cmdDump(vshControl *ctl, const vshCmd *cmd)
>  {
>      virDomainPtr dom;
> -    bool ret = false;
>      bool verbose = false;
>      const char *name = NULL;
>      const char *to = NULL;
> @@ -5556,12 +5555,12 @@ cmdDump(vshControl *ctl, const vshCmd *cmd)
>  
>      virThreadJoin(&workerThread);
>  
> -    if (!ret)
> +    if (!data.ret)
>          vshPrintExtra(ctl, _("\nDomain %s dumped to %s\n"), name, to);
>  
>   cleanup:
>      virshDomainFree(dom);
> -    return !ret;
> +    return !data.ret;
>  }

This is in the same vein as fbc4e81a36d1, and I think we should
definitely squeeze it in before release, so that's a

  Signed-off-by: Andrea Bolognani <abologna@redhat.com>

from me. Dan, what do you think?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] virsh: Fix return code for dump
Posted by Michal Privoznik 3 years, 10 months ago
On 5/4/20 2:09 PM, Andrea Bolognani wrote:
> On Mon, 2020-05-04 at 09:48 +0000, Xuyandong (Yandong Xu) wrote:
>> +++ b/tools/virsh-domain.c
>> @@ -5522,7 +5522,6 @@ static bool
>>   cmdDump(vshControl *ctl, const vshCmd *cmd)
>>   {
>>       virDomainPtr dom;
>> -    bool ret = false;
>>       bool verbose = false;
>>       const char *name = NULL;
>>       const char *to = NULL;
>> @@ -5556,12 +5555,12 @@ cmdDump(vshControl *ctl, const vshCmd *cmd)
>>   
>>       virThreadJoin(&workerThread);
>>   
>> -    if (!ret)
>> +    if (!data.ret)
>>           vshPrintExtra(ctl, _("\nDomain %s dumped to %s\n"), name, to);
>>   
>>    cleanup:
>>       virshDomainFree(dom);
>> -    return !ret;
>> +    return !data.ret;
>>   }
> 
> This is in the same vein as fbc4e81a36d1, and I think we should
> definitely squeeze it in before release, so that's a
> 
>    Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> 
> from me. Dan, what do you think?
> 

I'm in the middle of review too. But I find !data.ret a bit confusing. 
The function is returning a boolean and not a pointer or an int. So how 
about 'return data.ret != 0'? I know John was against using 'if (!int)' 
and it kind of makes sense.

Or even better, we can use our regular pattern and keep @ret, and do 
something like:

if (data.ret < 0)
   goto cleanup;

vshPrintExtra()
ret = true;
...
return ret;


This is something that is perfectly fixable by committer, so no need to 
resend. I too agree that this is something that needs fixing and the 
sooner we fix it the better.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [PATCH] virsh: Fix return code for dump
Posted by Andrea Bolognani 3 years, 10 months ago
On Mon, 2020-05-04 at 14:19 +0200, Michal Privoznik wrote:
> I'm in the middle of review too. But I find !data.ret a bit confusing. 
> The function is returning a boolean and not a pointer or an int. So how 
> about 'return data.ret != 0'? I know John was against using 'if (!int)' 
> and it kind of makes sense.
> 
> Or even better, we can use our regular pattern and keep @ret, and do 
> something like:
> 
> if (data.ret < 0)
>    goto cleanup;
> 
> vshPrintExtra()
> ret = true;
> ...
> return ret;

Yeah I don't like it either, but if you look at the rest of the file
there are multiple occurrences of the same pattern, which we should
address as well.

I will push the minimal fix for now, so that it can make it into
6.3.0; afterwards, someone can post a patch that removes the pattern
from the entire file.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] virsh: Fix return code for dump
Posted by Daniel P. Berrangé 3 years, 10 months ago
On Mon, May 04, 2020 at 02:09:02PM +0200, Andrea Bolognani wrote:
> On Mon, 2020-05-04 at 09:48 +0000, Xuyandong (Yandong Xu) wrote:
> > +++ b/tools/virsh-domain.c
> > @@ -5522,7 +5522,6 @@ static bool
> >  cmdDump(vshControl *ctl, const vshCmd *cmd)
> >  {
> >      virDomainPtr dom;
> > -    bool ret = false;
> >      bool verbose = false;
> >      const char *name = NULL;
> >      const char *to = NULL;
> > @@ -5556,12 +5555,12 @@ cmdDump(vshControl *ctl, const vshCmd *cmd)
> >  
> >      virThreadJoin(&workerThread);
> >  
> > -    if (!ret)
> > +    if (!data.ret)
> >          vshPrintExtra(ctl, _("\nDomain %s dumped to %s\n"), name, to);
> >  
> >   cleanup:
> >      virshDomainFree(dom);
> > -    return !ret;
> > +    return !data.ret;
> >  }
> 
> This is in the same vein as fbc4e81a36d1, and I think we should
> definitely squeeze it in before release, so that's a
> 
>   Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> 
> from me. Dan, what do you think?

Agreed,

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 :|