1. Patchew
  2. Libvirt
  3. View series

[PATCH] tools: virsh-snapshot: refactor small functions

Kristina Hanicova posted 1 patch 4 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/4967fc94b438a43062af0f3357e1cbd6f1220f94.1631884795.git.khanicov@redhat.com
tools/virsh-snapshot.c | 43 +++++++++++++++---------------------------
1 file changed, 15 insertions(+), 28 deletions(-)
[PATCH] tools: virsh-snapshot: refactor small functions
Posted by Kristina Hanicova 4 years, 4 months ago 
This patch includes:
* removal of dead code
* simplifying nested if conditions
* removal of unnecessary variables
* usage of "direct" boolean return

Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
---
 tools/virsh-snapshot.c | 43 +++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 2659b4340d..3bdad03df0 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -771,7 +771,6 @@ virshSnapshotFilter(vshControl *ctl, virDomainSnapshotPtr snapshot,
     g_autofree char *xml = NULL;
     g_autoptr(xmlDoc) xmldoc = NULL;
     g_autoptr(xmlXPathContext) ctxt = NULL;
-    int ret = -1;
     g_autofree char *state = NULL;
 
     if (!snapshot)
@@ -796,20 +795,17 @@ virshSnapshotFilter(vshControl *ctl, virDomainSnapshotPtr snapshot,
         return -1;
     }
     if (STREQ(state, "disk-snapshot")) {
-        ret = ((flags & (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
-                         VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)) ==
-               (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
-                VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL));
-    } else {
-        if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL))
-            ret = 0;
-        else if (STREQ(state, "shutoff"))
-            ret = !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_INACTIVE);
-        else
-            ret = !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE);
+        return ((flags & (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
+                          VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)) ==
+                (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
+                 VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL));
     }
 
-    return ret;
+    if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL))
+        return 0;
+    if (STREQ(state, "shutoff"))
+        return !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_INACTIVE);
+    return !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE);
 }
 
 /*
@@ -869,14 +865,8 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd)
      * attempt a fallback.  */
     current = virDomainSnapshotIsCurrent(snapshot, 0);
     if (current < 0) {
-        g_autoptr(virshDomainSnapshot) other = NULL;
-
         vshResetLibvirtError();
         current = 0;
-        if (other) {
-            if (STREQ(name, virDomainSnapshotGetName(other)))
-                current = 1;
-        }
     }
     vshPrint(ctl, "%-15s %s\n", _("Current:"),
              current > 0 ? _("yes") : _("no"));
@@ -1776,10 +1766,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
         vshResetLibvirtError();
         result = virDomainRevertToSnapshot(snapshot, flags);
     }
-    if (result < 0)
-        return false;
 
-    return true;
+    return result >= 0;
 }
 
 /*
@@ -1844,16 +1832,15 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd)
     /* XXX If we wanted, we could emulate DELETE_CHILDREN_ONLY even on
      * older servers that reject the flag, by manually computing the
      * list of descendants.  But that's a lot of code to maintain.  */
-    if (virDomainSnapshotDelete(snapshot, flags) == 0) {
-        if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)
-            vshPrintExtra(ctl, _("Domain snapshot %s children deleted\n"), name);
-        else
-            vshPrintExtra(ctl, _("Domain snapshot %s deleted\n"), name);
-    } else {
+    if (virDomainSnapshotDelete(snapshot, flags) < 0) {
         vshError(ctl, _("Failed to delete snapshot %s"), name);
         return false;
     }
 
+    if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)
+        vshPrintExtra(ctl, _("Domain snapshot %s children deleted\n"), name);
+    else
+        vshPrintExtra(ctl, _("Domain snapshot %s deleted\n"), name);
     return true;
 }
 
-- 
2.31.1
Re: [PATCH] tools: virsh-snapshot: refactor small functions
Posted by Michal Prívozník 4 years, 4 months ago 
On 9/17/21 3:23 PM, Kristina Hanicova wrote:
> This patch includes:
> * removal of dead code
> * simplifying nested if conditions
> * removal of unnecessary variables
> * usage of "direct" boolean return
> 
> Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
> ---
>  tools/virsh-snapshot.c | 43 +++++++++++++++---------------------------
>  1 file changed, 15 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index 2659b4340d..3bdad03df0 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -771,7 +771,6 @@ virshSnapshotFilter(vshControl *ctl, virDomainSnapshotPtr snapshot,
>      g_autofree char *xml = NULL;
>      g_autoptr(xmlDoc) xmldoc = NULL;
>      g_autoptr(xmlXPathContext) ctxt = NULL;
> -    int ret = -1;
>      g_autofree char *state = NULL;
>  
>      if (!snapshot)
> @@ -796,20 +795,17 @@ virshSnapshotFilter(vshControl *ctl, virDomainSnapshotPtr snapshot,
>          return -1;
>      }
>      if (STREQ(state, "disk-snapshot")) {
> -        ret = ((flags & (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
> -                         VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)) ==
> -               (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
> -                VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL));

So the only way this can be true is if both flags are set at the same
time. Since you're touching this, how about:

        return !!((flags & VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) &&
                  (flags & VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL));

I find it more readable. I'll change it before pushing.

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

Michal
Re: [PATCH] tools: virsh-snapshot: refactor small functions
Posted by Peter Krempa 4 years, 4 months ago 
On Mon, Sep 20, 2021 at 09:36:25 +0200, Michal Prívozník wrote:
> On 9/17/21 3:23 PM, Kristina Hanicova wrote:
> > This patch includes:
> > * removal of dead code
> > * simplifying nested if conditions
> > * removal of unnecessary variables
> > * usage of "direct" boolean return
> > 
> > Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
> > ---
> >  tools/virsh-snapshot.c | 43 +++++++++++++++---------------------------
> >  1 file changed, 15 insertions(+), 28 deletions(-)
> > 
> > diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> > index 2659b4340d..3bdad03df0 100644
> > --- a/tools/virsh-snapshot.c
> > +++ b/tools/virsh-snapshot.c
> > @@ -771,7 +771,6 @@ virshSnapshotFilter(vshControl *ctl, virDomainSnapshotPtr snapshot,
> >      g_autofree char *xml = NULL;
> >      g_autoptr(xmlDoc) xmldoc = NULL;
> >      g_autoptr(xmlXPathContext) ctxt = NULL;
> > -    int ret = -1;
> >      g_autofree char *state = NULL;
> >  
> >      if (!snapshot)
> > @@ -796,20 +795,17 @@ virshSnapshotFilter(vshControl *ctl, virDomainSnapshotPtr snapshot,
> >          return -1;
> >      }
> >      if (STREQ(state, "disk-snapshot")) {
> > -        ret = ((flags & (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
> > -                         VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)) ==
> > -               (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
> > -                VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL));
> 
> So the only way this can be true is if both flags are set at the same
> time. Since you're touching this, how about:
> 
>         return !!((flags & VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) &&
>                   (flags & VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL));

Note that the double negation trick to force a cast to boolean isn't
necessary here, since the result of

 (flags & VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) && (flags & VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)

is already a boolean (result of 'A && B')

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.