[PATCH v2 0/7] virsh: domain: small refactoring
This is partially v2 of: https://listman.redhat.com/archives/libvir-list/2021-September/msg00728.html Diff to v1: * split changes into smaller patches Kristina Hanicova (7): virsh: domain: use early return in virshDomainDefine() virsh: domain: remove nested 'if' in cmdAttachDisk() virsh: domain: remove 'ret' variable and use direct return when possible virsh: domain: remove 'ret' variable, use early return when possible virsh: domain: use early return when possible virsh: domain: remove 'ret' variable and use 'count' instead virsh: domain: remove else branch tools/virsh-domain.c | 232 +++++++++++++++++-------------------------- 1 file changed, 92 insertions(+), 140 deletions(-) -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index bee3346eb0..edee548d8a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -245,18 +245,18 @@ static virDomainPtr virshDomainDefine(virConnectPtr conn, const char *xml, unsigned int flags) { virDomainPtr dom; - if (flags) { - dom = virDomainDefineXMLFlags(conn, xml, flags); - /* If validate is the only flag, just drop it and - * try again. - */ - if (!dom) { - if ((virGetLastErrorCode() == VIR_ERR_NO_SUPPORT) && - (flags == VIR_DOMAIN_DEFINE_VALIDATE)) - dom = virDomainDefineXML(conn, xml); - } - } else { - dom = virDomainDefineXML(conn, xml); + + if (!flags) + return virDomainDefineXML(conn, xml); + + dom = virDomainDefineXMLFlags(conn, xml, flags); + /* If validate is the only flag, just drop it and + * try again. + */ + if (!dom) { + if ((virGetLastErrorCode() == VIR_ERR_NO_SUPPORT) && + (flags == VIR_DOMAIN_DEFINE_VALIDATE)) + dom = virDomainDefineXML(conn, xml); } return dom; } -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index edee548d8a..0d482f9e15 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -659,12 +659,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) return false; } - if (mode) { - if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { - vshError(ctl, _("No support for %s in command 'attach-disk'"), - mode); - return false; - } + if (mode && STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) { + vshError(ctl, _("No support for %s in command 'attach-disk'"), mode); + return false; } if (wwn && !virValidateWWN(wwn)) -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 62 ++++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 40 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0d482f9e15..1da9b183bf 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2702,7 +2702,6 @@ virshBlockJobAbort(virDomainPtr dom, static bool cmdBlockjob(vshControl *ctl, const vshCmd *cmd) { - bool ret = false; bool raw = vshCommandOptBool(cmd, "raw"); bool bytes = vshCommandOptBool(cmd, "bytes"); bool abortMode = vshCommandOptBool(cmd, "abort"); @@ -2735,13 +2734,10 @@ cmdBlockjob(vshControl *ctl, const vshCmd *cmd) return false; if (bandwidth) - ret = virshBlockJobSetSpeed(ctl, cmd, dom, path, bytes); - else if (abortMode || pivot || async) - ret = virshBlockJobAbort(dom, path, pivot, async); - else - ret = virshBlockJobInfo(ctl, dom, path, raw, bytes); - - return ret; + return virshBlockJobSetSpeed(ctl, cmd, dom, path, bytes); + if (abortMode || pivot || async) + return virshBlockJobAbort(dom, path, pivot, async); + return virshBlockJobInfo(ctl, dom, path, raw, bytes); } /* @@ -5063,7 +5059,6 @@ cmdSchedInfoUpdateOne(vshControl *ctl, const char *field, const char *value) { virTypedParameterPtr param; - int ret = -1; size_t i; for (i = 0; i < nsrc_params; i++) { @@ -5078,14 +5073,11 @@ cmdSchedInfoUpdateOne(vshControl *ctl, vshSaveLibvirtError(); return -1; } - ret = 0; - break; + return 0; } - if (ret < 0) - vshError(ctl, _("invalid scheduler option: %s"), field); - - return ret; + vshError(ctl, _("invalid scheduler option: %s"), field); + return -1; } static int @@ -5529,7 +5521,6 @@ virshGenFileName(vshControl *ctl, virDomainPtr dom, const char *mime) g_autoptr(GDateTime) now = g_date_time_new_now_local(); g_autofree char *nowstr = NULL; const char *ext = NULL; - char *ret = NULL; if (!dom) { vshError(ctl, "%s", _("Invalid domain supplied")); @@ -5544,10 +5535,8 @@ virshGenFileName(vshControl *ctl, virDomainPtr dom, const char *mime) nowstr = g_date_time_format(now, "%Y-%m-%d-%H:%M:%S"); - ret = g_strdup_printf("%s-%s%s", virDomainGetName(dom), - nowstr, NULLSTR_EMPTY(ext)); - - return ret; + return g_strdup_printf("%s-%s%s", virDomainGetName(dom), + nowstr, NULLSTR_EMPTY(ext)); } static bool @@ -5686,7 +5675,6 @@ static bool cmdSetLifecycleAction(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; - bool ret = true; bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); @@ -5727,10 +5715,9 @@ cmdSetLifecycleAction(vshControl *ctl, const vshCmd *cmd) if (virDomainSetLifecycleAction(dom, type, action, flags) < 0) { vshError(ctl, "%s", _("Unable to change lifecycle action.")); - ret = false; + return false; } - - return ret; + return true; } /* @@ -6469,15 +6456,14 @@ static bool cmdDomjobabort(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; - bool ret = true; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; if (virDomainAbortJob(dom) < 0) - ret = false; + return false; - return ret; + return true; } /* @@ -11926,12 +11912,12 @@ virshDomainDetachInterface(char *doc, xmlNodePtr cur = NULL, matchNode = NULL; g_autofree char *detach_xml = NULL; char buf[64]; - int diff_mac, ret = -1; + int diff_mac = -1; size_t i; if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) { vshError(ctl, "%s", _("Failed to get interface information")); - goto cleanup; + return false; } g_snprintf(buf, sizeof(buf), "/domain/devices/interface[@type='%s']", type); @@ -11939,13 +11925,13 @@ virshDomainDetachInterface(char *doc, if (obj == NULL || obj->type != XPATH_NODESET || obj->nodesetval == NULL || obj->nodesetval->nodeNr == 0) { vshError(ctl, _("No interface found whose type is %s"), type); - goto cleanup; + return false; } if (!mac && obj->nodesetval->nodeNr > 1) { vshError(ctl, _("Domain has %d interfaces. Please specify which one " "to detach using --mac"), obj->nodesetval->nodeNr); - goto cleanup; + return false; } if (!mac) { @@ -11968,7 +11954,7 @@ virshDomainDetachInterface(char *doc, "MAC address %s. You must use detach-device and " "specify the device pci address to remove it."), mac); - goto cleanup; + return false; } matchNode = obj->nodesetval->nodeTab[i]; } @@ -11978,22 +11964,18 @@ virshDomainDetachInterface(char *doc, } if (!matchNode) { vshError(ctl, _("No interface with MAC address %s was found"), mac); - goto cleanup; + return false; } hit: if (!(detach_xml = virXMLNodeToString(xml, matchNode))) { vshSaveLibvirtError(); - goto cleanup; + return false; } if (flags != 0 || current) - ret = virDomainDetachDeviceFlags(dom, detach_xml, flags); - else - ret = virDomainDetachDevice(dom, detach_xml); - - cleanup: - return ret == 0; + return virDomainDetachDeviceFlags(dom, detach_xml, flags) == 0; + return virDomainDetachDevice(dom, detach_xml) == 0; } -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 84 +++++++++++++++++--------------------------- 1 file changed, 33 insertions(+), 51 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1da9b183bf..1692197d1a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2923,7 +2923,6 @@ cmdBlockresize(vshControl *ctl, const vshCmd *cmd) const char *path = NULL; unsigned long long size = 0; unsigned int flags = 0; - bool ret = false; if (vshCommandOptStringReq(ctl, cmd, "path", (const char **) &path) < 0) return false; @@ -2942,12 +2941,11 @@ cmdBlockresize(vshControl *ctl, const vshCmd *cmd) if (virDomainBlockResize(dom, path, size, flags) < 0) { vshError(ctl, _("Failed to resize block device '%s'"), path); - } else { - vshPrintExtra(ctl, _("Block device '%s' is resized"), path); - ret = true; + return false; } - return ret; + vshPrintExtra(ctl, _("Block device '%s' is resized"), path); + return true; } #ifndef WIN32 @@ -3421,19 +3419,17 @@ cmdSuspend(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; const char *name; - bool ret = true; if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) return false; - if (virDomainSuspend(dom) == 0) { - vshPrintExtra(ctl, _("Domain '%s' suspended\n"), name); - } else { + if (virDomainSuspend(dom) != 0) { vshError(ctl, _("Failed to suspend domain '%s'"), name); - ret = false; + return false; } - return ret; + vshPrintExtra(ctl, _("Domain '%s' suspended\n"), name); + return true; } /* @@ -5802,20 +5798,18 @@ static bool cmdResume(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; - bool ret = true; const char *name; if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) return false; - if (virDomainResume(dom) == 0) { - vshPrintExtra(ctl, _("Domain '%s' resumed\n"), name); - } else { + if (virDomainResume(dom) != 0) { vshError(ctl, _("Failed to resume domain '%s'"), name); - ret = false; + return false; } - return ret; + vshPrintExtra(ctl, _("Domain '%s' resumed\n"), name); + return true; } /* @@ -5997,20 +5991,18 @@ static bool cmdReset(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; - bool ret = true; const char *name; if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) return false; - if (virDomainReset(dom, 0) == 0) { - vshPrintExtra(ctl, _("Domain '%s' was reset\n"), name); - } else { + if (virDomainReset(dom, 0) != 0) { vshError(ctl, _("Failed to reset domain '%s'"), name); - ret = false; + return false; } - return ret; + vshPrintExtra(ctl, _("Domain '%s' was reset\n"), name); + return true; } /* @@ -8183,7 +8175,6 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; const char *from = NULL; - bool ret = true; char *buffer; unsigned int flags = 0; virshControl *priv = ctl->privData; @@ -8203,14 +8194,14 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd) dom = virDomainDefineXML(priv->conn, buffer); VIR_FREE(buffer); - if (dom != NULL) { - vshPrintExtra(ctl, _("Domain '%s' defined from %s\n"), - virDomainGetName(dom), from); - } else { + if (!dom) { vshError(ctl, _("Failed to define domain from %s"), from); - ret = false; + return false; } - return ret; + + vshPrintExtra(ctl, _("Domain '%s' defined from %s\n"), + virDomainGetName(dom), from); + return true; } /* @@ -8239,7 +8230,6 @@ static bool cmdDestroy(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; - bool ret = true; const char *name; unsigned int flags = 0; int result; @@ -8255,14 +8245,13 @@ cmdDestroy(vshControl *ctl, const vshCmd *cmd) else result = virDomainDestroy(dom); - if (result == 0) { - vshPrintExtra(ctl, _("Domain '%s' destroyed\n"), name); - } else { + if (result < 0) { vshError(ctl, _("Failed to destroy domain '%s'"), name); - ret = false; + return false; } - return ret; + vshPrintExtra(ctl, _("Domain '%s' destroyed\n"), name); + return true; } /* @@ -9955,7 +9944,6 @@ static bool cmdDumpXML(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; - bool ret = true; g_autofree char *dump = NULL; unsigned int flags = 0; bool inactive = vshCommandOptBool(cmd, "inactive"); @@ -9975,14 +9963,11 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - dump = virDomainGetXMLDesc(dom, flags); - if (dump != NULL) { - vshPrint(ctl, "%s", dump); - } else { - ret = false; - } + if (!(dump = virDomainGetXMLDesc(dom, flags))) + return false; - return ret; + vshPrint(ctl, "%s", dump); + return true; } /* @@ -10016,7 +10001,6 @@ static const vshCmdOptDef opts_domxmlfromnative[] = { static bool cmdDomXMLFromNative(vshControl *ctl, const vshCmd *cmd) { - bool ret = true; const char *format = NULL; const char *configFile = NULL; g_autofree char *configData = NULL; @@ -10032,13 +10016,11 @@ cmdDomXMLFromNative(vshControl *ctl, const vshCmd *cmd) return false; xmlData = virConnectDomainXMLFromNative(priv->conn, format, configData, flags); - if (xmlData != NULL) { - vshPrint(ctl, "%s", xmlData); - } else { - ret = false; - } + if (!xmlData) + return false; - return ret; + vshPrint(ctl, "%s", xmlData); + return true; } /* -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1692197d1a..3c496d845a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5883,13 +5883,13 @@ cmdShutdown(vshControl *ctl, const vshCmd *cmd) rv = virDomainShutdownFlags(dom, flags); else rv = virDomainShutdown(dom); - if (rv == 0) { - vshPrintExtra(ctl, _("Domain '%s' is being shutdown\n"), name); - } else { + + if (rv != 0) { vshError(ctl, _("Failed to shutdown domain '%s'"), name); return false; } + vshPrintExtra(ctl, _("Domain '%s' is being shutdown\n"), name); return true; } @@ -5959,13 +5959,12 @@ cmdReboot(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) return false; - if (virDomainReboot(dom, flags) == 0) { - vshPrintExtra(ctl, _("Domain '%s' is being rebooted\n"), name); - } else { + if (virDomainReboot(dom, flags) != 0) { vshError(ctl, _("Failed to reboot domain '%s'"), name); return false; } + vshPrintExtra(ctl, _("Domain '%s' is being rebooted\n"), name); return true; } -- 2.31.1
This patch also includes use of an early return in case of an error. I think the changes make the functions more readable. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3c496d845a..94ed786751 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13608,10 +13608,10 @@ static bool cmdDomFSFreeze(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; - int ret = -1; const vshCmdOpt *opt = NULL; g_autofree const char **mountpoints = NULL; size_t nmountpoints = 0; + int count = 0; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -13621,16 +13621,13 @@ cmdDomFSFreeze(vshControl *ctl, const vshCmd *cmd) mountpoints[nmountpoints-1] = opt->data; } - ret = virDomainFSFreeze(dom, mountpoints, nmountpoints, 0); - if (ret < 0) { + if ((count = virDomainFSFreeze(dom, mountpoints, nmountpoints, 0)) < 0) { vshError(ctl, _("Unable to freeze filesystems")); - goto cleanup; + return false; } - vshPrintExtra(ctl, _("Froze %d filesystem(s)\n"), ret); - - cleanup: - return ret >= 0; + vshPrintExtra(ctl, _("Froze %d filesystem(s)\n"), count); + return true; } static const vshCmdInfo info_domfsthaw[] = { @@ -13656,10 +13653,10 @@ static bool cmdDomFSThaw(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; - int ret = -1; const vshCmdOpt *opt = NULL; g_autofree const char **mountpoints = NULL; size_t nmountpoints = 0; + int count = 0; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -13669,16 +13666,13 @@ cmdDomFSThaw(vshControl *ctl, const vshCmd *cmd) mountpoints[nmountpoints-1] = opt->data; } - ret = virDomainFSThaw(dom, mountpoints, nmountpoints, 0); - if (ret < 0) { + if ((count = virDomainFSThaw(dom, mountpoints, nmountpoints, 0)) < 0) { vshError(ctl, _("Unable to thaw filesystems")); - goto cleanup; + return false; } - vshPrintExtra(ctl, _("Thawed %d filesystem(s)\n"), ret); - - cleanup: - return ret >= 0; + vshPrintExtra(ctl, _("Thawed %d filesystem(s)\n"), count); + return true; } static const vshCmdInfo info_domfsinfo[] = { -- 2.31.1
I removed else branches after return/break as they are not necessary and the code looks cleaner without them. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 94ed786751..ca8f08d949 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11046,12 +11046,12 @@ cmdMigrateCompCache(vshControl *ctl, const vshCmd *cmd) return false; rc = vshCommandOptULongLong(ctl, cmd, "size", &size); - if (rc < 0) { + if (rc < 0) + return false; + + if (rc != 0 && + (virDomainMigrateSetCompressionCache(dom, size, 0) < 0)) return false; - } else if (rc != 0) { - if (virDomainMigrateSetCompressionCache(dom, size, 0) < 0) - return false; - } if (virDomainMigrateGetCompressionCache(dom, &size, 0) < 0) return false; @@ -11432,11 +11432,9 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) /* We got what we came for so return successfully */ ret = true; - if (!all) { + if (!all) break; - } else { - vshPrint(ctl, "\n"); - } + vshPrint(ctl, "\n"); } if (!ret) { -- 2.31.1
On 9/24/21 5:17 PM, Kristina Hanicova wrote:
This is partially v2 of: https://listman.redhat.com/archives/libvir-list/2021-September/msg00728.html
Diff to v1: * split changes into smaller patches
Kristina Hanicova (7): virsh: domain: use early return in virshDomainDefine() virsh: domain: remove nested 'if' in cmdAttachDisk() virsh: domain: remove 'ret' variable and use direct return when possible virsh: domain: remove 'ret' variable, use early return when possible virsh: domain: use early return when possible virsh: domain: remove 'ret' variable and use 'count' instead virsh: domain: remove else branch
tools/virsh-domain.c | 232 +++++++++++++++++-------------------------- 1 file changed, 92 insertions(+), 140 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Kristina Hanicova -
Michal Prívozník