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

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

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

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')
participants (3)
-
Kristina Hanicova
-
Michal Prívozník
-
Peter Krempa