tools/virsh-snapshot.c | 43 +++++++++++++++--------------------------- 1 file changed, 15 insertions(+), 28 deletions(-)
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')
© 2016 - 2024 Red Hat, Inc.