[PATCH 0/5] virsh: small refactoring
This series targets some alterations to make the code more readable and consistent. Kristina Hanicova (5): virsh: checkpoint & domain-monitor: small refactoring virsh: domain: refactoring of small functions virsh: domain: change the way functions return bool value virsh: domain, host & volume: refactor bigger functions virsh: refactor functions to not use 'ret' variable tools/virsh-checkpoint.c | 19 +- tools/virsh-domain-monitor.c | 314 +++++++++---------- tools/virsh-domain.c | 567 ++++++++++++++--------------------- tools/virsh-host.c | 166 +++++----- tools/virsh-interface.c | 65 ++-- tools/virsh-network.c | 47 ++- tools/virsh-nodedev.c | 32 +- tools/virsh-nwfilter.c | 15 +- tools/virsh-util.c | 3 +- tools/virsh-volume.c | 56 ++-- 10 files changed, 546 insertions(+), 738 deletions(-) -- 2.31.1
This patch includes small refactoring such as: * early return in case of an error - helps with indentation * removal of 'else' branch after return - unnecessary * altering code to be more consistent with the rest of the file - function calls inside of parentheses, etc. * removal of unnecessary variables - mainly the ones used for return value instead of returning it directly * missing parentheses around multi-line block of code Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-checkpoint.c | 19 +-- tools/virsh-domain-monitor.c | 314 ++++++++++++++++------------------- 2 files changed, 155 insertions(+), 178 deletions(-) diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index 8ad37ece69..c1a9e66a24 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -292,12 +292,12 @@ virshLookupCheckpoint(vshControl *ctl, if (vshCommandOptStringReq(ctl, cmd, arg, &chkname) < 0) return -1; - if (chkname) { - *chk = virDomainCheckpointLookupByName(dom, chkname, 0); - } else { + if (!chkname) { vshError(ctl, _("--%s is required"), arg); return -1; } + + *chk = virDomainCheckpointLookupByName(dom, chkname, 0); if (!*chk) { vshReportError(ctl); return -1; @@ -931,13 +931,13 @@ cmdCheckpointParent(vshControl *ctl, if (virshGetCheckpointParent(ctl, checkpoint, &parent) < 0) return false; + if (!parent) { vshError(ctl, _("checkpoint '%s' has no parent"), name); return false; } vshPrint(ctl, "%s", parent); - return true; } @@ -1002,16 +1002,15 @@ cmdCheckpointDelete(vshControl *ctl, if (vshCommandOptBool(cmd, "metadata")) flags |= VIR_DOMAIN_CHECKPOINT_DELETE_METADATA_ONLY; - if (virDomainCheckpointDelete(checkpoint, flags) == 0) { - if (flags & VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN_ONLY) - vshPrintExtra(ctl, _("Domain checkpoint %s children deleted\n"), name); - else - vshPrintExtra(ctl, _("Domain checkpoint %s deleted\n"), name); - } else { + if (virDomainCheckpointDelete(checkpoint, flags) < 0) { vshError(ctl, _("Failed to delete checkpoint %s"), name); return false; } + if (flags & VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN_ONLY) + vshPrintExtra(ctl, _("Domain checkpoint %s children deleted\n"), name); + else + vshPrintExtra(ctl, _("Domain checkpoint %s deleted\n"), name); return true; } diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index eb3e0ef11a..d9bc869080 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -58,6 +58,7 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool title, g_autoptr(xmlDoc) doc = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; int type; + int errCode; if (title) type = VIR_DOMAIN_METADATA_TITLE; @@ -66,19 +67,18 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool title, if ((desc = virDomainGetMetadata(dom, type, NULL, flags))) { return desc; - } else { - int errCode = virGetLastErrorCode(); - - if (errCode == VIR_ERR_NO_DOMAIN_METADATA) { - desc = g_strdup(""); - vshResetLibvirtError(); - return desc; - } + } + errCode = virGetLastErrorCode(); - if (errCode != VIR_ERR_NO_SUPPORT) - return desc; + if (errCode == VIR_ERR_NO_DOMAIN_METADATA) { + desc = g_strdup(""); + vshResetLibvirtError(); + return desc; } + if (errCode != VIR_ERR_NO_SUPPORT) + return desc; + /* fall back to xml */ if (virshDomainGetXMLFromDom(ctl, dom, flags, &doc, &ctxt) < 0) return NULL; @@ -467,79 +467,72 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) human = vshCommandOptBool(cmd, "human"); - if (all) { - bool active = virDomainIsActive(dom) == 1; - int rc; + if (!all) { + g_autofree char *cap = NULL; + g_autofree char *alloc = NULL; + g_autofree char *phy = NULL; - if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0) + if (virDomainGetBlockInfo(dom, device, &info, 0) < 0) return false; - ndisks = virXPathNodeSet("./devices/disk", ctxt, &disks); - if (ndisks < 0) + if (!cmdDomblkinfoGet(&info, &cap, &alloc, &phy, human)) return false; - /* title */ - table = vshTableNew(_("Target"), _("Capacity"), _("Allocation"), _("Physical"), NULL); - if (!table) - return false; + vshPrint(ctl, "%-15s %s\n", _("Capacity:"), cap); + vshPrint(ctl, "%-15s %s\n", _("Allocation:"), alloc); + vshPrint(ctl, "%-15s %s\n", _("Physical:"), phy); + return true; + } - for (i = 0; i < ndisks; i++) { - g_autofree char *target = NULL; - g_autofree char *protocol = NULL; - g_autofree char *cap = NULL; - g_autofree char *alloc = NULL; - g_autofree char *phy = NULL; - - ctxt->node = disks[i]; - protocol = virXPathString("string(./source/@protocol)", ctxt); - target = virXPathString("string(./target/@dev)", ctxt); - - if (virXPathBoolean("boolean(./source)", ctxt) == 1) { - - rc = virDomainGetBlockInfo(dom, target, &info, 0); - - if (rc < 0) { - /* If protocol is present that's an indication of a - * networked storage device which cannot provide statistics, - * so generate 0 based data and get the next disk. */ - if (protocol && !active && - virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR && - virGetLastErrorDomain() == VIR_FROM_STORAGE) { - memset(&info, 0, sizeof(info)); - vshResetLibvirtError(); - } else { - return false; - } - } - } else { - /* if we don't call virDomainGetBlockInfo() who clears 'info' - * we have to do it manually */ - memset(&info, 0, sizeof(info)); - } + if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0) + return false; - if (!cmdDomblkinfoGet(&info, &cap, &alloc, &phy, human)) - return false; - if (vshTableRowAppend(table, target, cap, alloc, phy, NULL) < 0) - return false; - } + ndisks = virXPathNodeSet("./devices/disk", ctxt, &disks); + if (ndisks < 0) + return false; - vshTablePrintToStdout(table, ctl); + /* title */ + table = vshTableNew(_("Target"), _("Capacity"), _("Allocation"), _("Physical"), NULL); + if (!table) + return false; - } else { + for (i = 0; i < ndisks; i++) { + g_autofree char *target = NULL; + g_autofree char *protocol = NULL; g_autofree char *cap = NULL; g_autofree char *alloc = NULL; g_autofree char *phy = NULL; - if (virDomainGetBlockInfo(dom, device, &info, 0) < 0) - return false; + ctxt->node = disks[i]; + protocol = virXPathString("string(./source/@protocol)", ctxt); + target = virXPathString("string(./target/@dev)", ctxt); + + if (virXPathBoolean("boolean(./source)", ctxt) == 1) { + if (virDomainGetBlockInfo(dom, target, &info, 0) < 0) { + /* If protocol is present that's an indication of a + * networked storage device which cannot provide statistics, + * so generate 0 based data and get the next disk. */ + if (!protocol || (virDomainIsActive(dom) != 1) || + virGetLastErrorCode() != VIR_ERR_INTERNAL_ERROR || + virGetLastErrorDomain() != VIR_FROM_STORAGE) + return false; + + memset(&info, 0, sizeof(info)); + vshResetLibvirtError(); + } + } else { + /* if we don't call virDomainGetBlockInfo() who clears 'info' + * we have to do it manually */ + memset(&info, 0, sizeof(info)); + } if (!cmdDomblkinfoGet(&info, &cap, &alloc, &phy, human)) return false; - vshPrint(ctl, "%-15s %s\n", _("Capacity:"), cap); - vshPrint(ctl, "%-15s %s\n", _("Allocation:"), alloc); - vshPrint(ctl, "%-15s %s\n", _("Physical:"), phy); + if (vshTableRowAppend(table, target, cap, alloc, phy, NULL) < 0) + return false; } + vshTablePrintToStdout(table, ctl); return true; } @@ -584,8 +577,6 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "inactive")) flags |= VIR_DOMAIN_XML_INACTIVE; - details = vshCommandOptBool(cmd, "details"); - if (virshDomainGetXML(ctl, cmd, flags, &xmldoc, &ctxt) < 0) return false; @@ -593,7 +584,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) if (ndisks < 0) return false; - if (details) + if ((details = vshCommandOptBool(cmd, "details"))) table = vshTableNew(_("Type"), _("Device"), _("Target"), _("Source"), NULL); else table = vshTableNew(_("Target"), _("Source"), NULL); @@ -618,8 +609,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) } } - target = virXPathString("string(./target/@dev)", ctxt); - if (!target) { + if (!(target = virXPathString("string(./target/@dev)", ctxt))) { vshError(ctl, "unable to query block list"); return false; } @@ -648,19 +638,16 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) "|./source/@path)", ctxt); } - if (details) { - if (vshTableRowAppend(table, type, device, target, - NULLSTR_MINUS(source), NULL) < 0) - return false; - } else { - if (vshTableRowAppend(table, target, - NULLSTR_MINUS(source), NULL) < 0) - return false; - } + if (details && (vshTableRowAppend(table, type, device, target, + NULLSTR_MINUS(source), NULL) < 0)) + return false; + + if (!details && (vshTableRowAppend(table, target, + NULLSTR_MINUS(source), NULL) < 0)) + return false; } vshTablePrintToStdout(table, ctl); - return true; } @@ -808,16 +795,15 @@ cmdDomIfGetLink(vshControl *ctl, const vshCmd *cmd) return false; } - if (ninterfaces < 1) { - if (macstr[0]) + if (ninterfaces != 1) { + if (ninterfaces > 1) + vshError(ctl, _("multiple matching interfaces found")); + else if (macstr[0]) vshError(ctl, _("Interface (mac: %s) not found."), macstr); else vshError(ctl, _("Interface (dev: %s) not found."), iface); return false; - } else if (ninterfaces > 1) { - vshError(ctl, _("multiple matching interfaces found")); - return false; } ctxt->node = interfaces[0]; @@ -952,8 +938,7 @@ cmdDomblkstat(vshControl *ctl, const vshCmd *cmd) virDomainBlockStatsStruct stats; g_autofree virTypedParameterPtr params = NULL; virTypedParameterPtr par = NULL; - const char *field = NULL; - int rc, nparams = 0; + int nparams = 0; size_t i; bool human = vshCommandOptBool(cmd, "human"); /* human readable output */ @@ -970,13 +955,11 @@ cmdDomblkstat(vshControl *ctl, const vshCmd *cmd) if (!device) device = ""; - rc = virDomainBlockStatsFlags(dom, device, NULL, &nparams, 0); - /* It might fail when virDomainBlockStatsFlags is not * supported on older libvirt, fallback to use virDomainBlockStats * then. */ - if (rc < 0) { + if (virDomainBlockStatsFlags(dom, device, NULL, &nparams, 0) < 0) { /* try older API if newer is not supported */ if (last_error->code != VIR_ERR_NO_SUPPORT) return false; @@ -1001,57 +984,58 @@ cmdDomblkstat(vshControl *ctl, const vshCmd *cmd) DOMBLKSTAT_LEGACY_PRINT(2, stats.wr_req); DOMBLKSTAT_LEGACY_PRINT(3, stats.wr_bytes); DOMBLKSTAT_LEGACY_PRINT(4, stats.errs); - } else { - params = g_new0(virTypedParameter, nparams); - if (virDomainBlockStatsFlags(dom, device, params, &nparams, 0) < 0) { - vshError(ctl, _("Failed to get block stats for domain '%s' device '%s'"), name, device); - return false; - } + return true; + } - /* set for prettier output */ - if (human) { - vshPrint(ctl, N_("Device: %s\n"), device); - device = ""; - } + params = g_new0(virTypedParameter, nparams); + if (virDomainBlockStatsFlags(dom, device, params, &nparams, 0) < 0) { + vshError(ctl, _("Failed to get block stats for domain '%s' device '%s'"), name, device); + return false; + } - /* at first print all known values in desired order */ - for (i = 0; domblkstat_output[i].field != NULL; i++) { - g_autofree char *value = NULL; + /* set for prettier output */ + if (human) { + vshPrint(ctl, N_("Device: %s\n"), device); + device = ""; + } - if (!(par = virTypedParamsGet(params, nparams, - domblkstat_output[i].field))) - continue; + /* at first print all known values in desired order */ + for (i = 0; domblkstat_output[i].field != NULL; i++) { + g_autofree char *value = NULL; + const char *field = NULL; - value = vshGetTypedParamValue(ctl, par); + if (!(par = virTypedParamsGet(params, nparams, + domblkstat_output[i].field))) + continue; - /* to print other not supported fields, mark the already printed */ - par->field[0] = '\0'; /* set the name to empty string */ + value = vshGetTypedParamValue(ctl, par); - /* translate into human readable or legacy spelling */ - field = NULL; - if (human) - field = _(domblkstat_output[i].human); - else - field = domblkstat_output[i].legacy; + /* to print other not supported fields, mark the already printed */ + par->field[0] = '\0'; /* set the name to empty string */ - /* use the provided spelling if no translation is available */ - if (!field) - field = domblkstat_output[i].field; + /* translate into human readable or legacy spelling */ + if (human) + field = domblkstat_output[i].human; + else + field = domblkstat_output[i].legacy; - vshPrint(ctl, "%s %-*s %s\n", device, - human ? 31 : 0, field, value); - } + /* use the provided spelling if no translation is available */ + if (!field) + field = domblkstat_output[i].field; + + vshPrint(ctl, "%s %-*s %s\n", device, + human ? 31 : 0, field, value); + } - /* go through the fields again, for remaining fields */ - for (i = 0; i < nparams; i++) { - g_autofree char *value = NULL; + /* go through the fields again, for remaining fields */ + for (i = 0; i < nparams; i++) { + g_autofree char *value = NULL; - if (!*params[i].field) - continue; + if (!*params[i].field) + continue; - value = vshGetTypedParamValue(ctl, params+i); - vshPrint(ctl, "%s %s %s\n", device, params[i].field, value); - } + value = vshGetTypedParamValue(ctl, params+i); + vshPrint(ctl, "%s %s %s\n", device, params[i].field, value); } return true; @@ -1292,11 +1276,9 @@ cmdDominfo(vshControl *ctl, const vshCmd *cmd) /* Security model and label information */ memset(&secmodel, 0, sizeof(secmodel)); if (virNodeGetSecurityModel(priv->conn, &secmodel) == -1) { - if (last_error->code != VIR_ERR_NO_SUPPORT) { + if (last_error->code != VIR_ERR_NO_SUPPORT) return false; - } else { - vshResetLibvirtError(); - } + vshResetLibvirtError(); } else { /* Only print something if a security model is active */ if (secmodel.model[0] != '\0') { @@ -1309,10 +1291,11 @@ cmdDominfo(vshControl *ctl, const vshCmd *cmd) if (virDomainGetSecurityLabel(dom, seclabel) == -1) { VIR_FREE(seclabel); return false; - } else { - if (seclabel->label[0] != '\0') - vshPrint(ctl, "%-15s %s (%s)\n", _("Security label:"), - seclabel->label, seclabel->enforcing ? "enforcing" : "permissive"); + } + + if (seclabel->label[0] != '\0') { + vshPrint(ctl, "%-15s %s (%s)\n", _("Security label:"), + seclabel->label, seclabel->enforcing ? "enforcing" : "permissive"); } VIR_FREE(seclabel); @@ -1421,7 +1404,6 @@ cmdDomTime(vshControl *ctl, const vshCmd *cmd) long long seconds = 0; unsigned int nseconds = 0; unsigned int flags = 0; - bool doSet = false; int rv; VSH_EXCLUSIVE_OPTIONS("time", "now"); @@ -1433,15 +1415,12 @@ cmdDomTime(vshControl *ctl, const vshCmd *cmd) rv = vshCommandOptLongLong(ctl, cmd, "time", &seconds); - if (rv < 0) { - /* invalid integer format */ + /* invalid integer format */ + if (rv < 0) return false; - } else if (rv > 0) { - /* valid integer to set */ - doSet = true; - } - if (doSet || now || rtcSync) { + /* valid integer to set */ + if (rv > 0 || now || rtcSync) { if (now && ((seconds = time(NULL)) == (time_t) -1)) { vshError(ctl, _("Unable to get current time")); return false; @@ -1453,21 +1432,22 @@ cmdDomTime(vshControl *ctl, const vshCmd *cmd) if (virDomainSetTime(dom, seconds, nseconds, flags) < 0) return false; - } else { - if (virDomainGetTime(dom, &seconds, &nseconds, flags) < 0) - return false; + return true; + } - if (pretty) { - g_autoptr(GDateTime) then = NULL; - g_autofree char *thenstr = NULL; + if (virDomainGetTime(dom, &seconds, &nseconds, flags) < 0) + return false; - then = g_date_time_new_from_unix_utc(seconds); - thenstr = g_date_time_format(then, "%Y-%m-%d %H:%M:%S"); + if (pretty) { + g_autoptr(GDateTime) then = NULL; + g_autofree char *thenstr = NULL; - vshPrint(ctl, _("Time: %s"), thenstr); - } else { - vshPrint(ctl, _("Time: %lld"), seconds); - } + then = g_date_time_new_from_unix_utc(seconds); + thenstr = g_date_time_format(then, "%Y-%m-%d %H:%M:%S"); + + vshPrint(ctl, _("Time: %s"), thenstr); + } else { + vshPrint(ctl, _("Time: %lld"), seconds); } return true; @@ -1508,17 +1488,15 @@ virshDomainSorter(const void *a, const void *b) if (ida == inactive && idb == inactive) return vshStrcasecmp(virDomainGetName(*da), virDomainGetName(*db)); - if (ida != inactive && idb != inactive) { + if (ida != inactive && idb != inactive && ida != idb) { if (ida > idb) return 1; - else if (ida < idb) - return -1; + return -1; } if (ida != inactive) return -1; - else - return 1; + return 1; } struct virshDomainList { @@ -1994,10 +1972,9 @@ cmdList(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%s%s", sep, id_buf); sep = " "; } - if (optName) { + if (optName) vshPrint(ctl, "%s%s", sep, virDomainGetName(dom)); - sep = " "; - } + vshPrint(ctl, "\n"); } } @@ -2372,13 +2349,14 @@ cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd) ip_addr_str = g_strdup(""); /* Don't repeat interface name */ - if (full || !j) + if (full || !j) { vshPrint(ctl, " %-10s %-17s %s\n", iface->name, NULLSTR_EMPTY(iface->hwaddr), ip_addr_str); - else + } else { vshPrint(ctl, " %-10s %-17s %s\n", "-", "-", ip_addr_str); + } } } -- 2.31.1
On Thu, Sep 23, 2021 at 17:08:00 +0200, Kristina Hanicova wrote:
This patch includes small refactoring such as: * early return in case of an error - helps with indentation * removal of 'else' branch after return - unnecessary * altering code to be more consistent with the rest of the file - function calls inside of parentheses, etc. * removal of unnecessary variables - mainly the ones used for return value instead of returning it directly * missing parentheses around multi-line block of code
There's a bit too much going on in this patch. It definitely goes against the guidance to make patches small, self contained and easy to review.
On a Thursday in 2021, Kristina Hanicova wrote:
This patch includes small refactoring such as: * early return in case of an error - helps with indentation * removal of 'else' branch after return - unnecessary * altering code to be more consistent with the rest of the file - function calls inside of parentheses, etc. * removal of unnecessary variables - mainly the ones used for return value instead of returning it directly * missing parentheses around multi-line block of code
There's too much going on in one patch at the same time. Please either do one kind of change in a patch, or split it per-function. And changes that result in moving lots of lines to a different indentation level are easier to read if they are separate from other changes.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-checkpoint.c | 19 +-- tools/virsh-domain-monitor.c | 314 ++++++++++++++++------------------- 2 files changed, 155 insertions(+), 178 deletions(-)
diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index 8ad37ece69..c1a9e66a24 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -931,13 +931,13 @@ cmdCheckpointParent(vshControl *ctl,
if (virshGetCheckpointParent(ctl, checkpoint, &parent) < 0) return false; + if (!parent) { vshError(ctl, _("checkpoint '%s' has no parent"), name); return false; }
vshPrint(ctl, "%s", parent); -
I was asked to leave an empty line before the final return by a reviewer recently, so it seems some people like it.
return true; }
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index eb3e0ef11a..d9bc869080 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -58,6 +58,7 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool title, g_autoptr(xmlDoc) doc = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; int type; + int errCode;
if (title) type = VIR_DOMAIN_METADATA_TITLE; @@ -66,19 +67,18 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool title,
if ((desc = virDomainGetMetadata(dom, type, NULL, flags))) { return desc; - } else { - int errCode = virGetLastErrorCode(); - - if (errCode == VIR_ERR_NO_DOMAIN_METADATA) { - desc = g_strdup(""); - vshResetLibvirtError(); - return desc; - } + } + errCode = virGetLastErrorCode();
- if (errCode != VIR_ERR_NO_SUPPORT) - return desc; + if (errCode == VIR_ERR_NO_DOMAIN_METADATA) { + desc = g_strdup(""); + vshResetLibvirtError(); + return desc;
You can return g_strdup("") directly.
}
+ if (errCode != VIR_ERR_NO_SUPPORT) + return desc;
Another alternative could be to split the fallback code below into a separate function and call it if errCode == NO_SUPPORT
+ /* fall back to xml */ if (virshDomainGetXMLFromDom(ctl, dom, flags, &doc, &ctxt) < 0) return NULL; @@ -467,79 +467,72 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
The changes in cmdDomblkinfo deserve their own patch.
human = vshCommandOptBool(cmd, "human");
- if (all) { - bool active = virDomainIsActive(dom) == 1; - int rc; + if (!all) {
[...]
@@ -584,8 +577,6 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "inactive")) flags |= VIR_DOMAIN_XML_INACTIVE;
- details = vshCommandOptBool(cmd, "details"); - if (virshDomainGetXML(ctl, cmd, flags, &xmldoc, &ctxt) < 0) return false;
@@ -593,7 +584,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) if (ndisks < 0) return false;
- if (details) + if ((details = vshCommandOptBool(cmd, "details")))
I prefer the version where we process the command options first and initialize the bool separately.
table = vshTableNew(_("Type"), _("Device"), _("Target"), _("Source"), NULL); else table = vshTableNew(_("Target"), _("Source"), NULL); @@ -618,8 +609,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) } }
- target = virXPathString("string(./target/@dev)", ctxt); - if (!target) { + if (!(target = virXPathString("string(./target/@dev)", ctxt))) { vshError(ctl, "unable to query block list"); return false; } @@ -648,19 +638,16 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd) "|./source/@path)", ctxt); }
- if (details) { - if (vshTableRowAppend(table, type, device, target, - NULLSTR_MINUS(source), NULL) < 0) - return false; - } else { - if (vshTableRowAppend(table, target, - NULLSTR_MINUS(source), NULL) < 0) - return false; - } + if (details && (vshTableRowAppend(table, type, device, target, + NULLSTR_MINUS(source), NULL) < 0)) + return false; + + if (!details && (vshTableRowAppend(table, target, + NULLSTR_MINUS(source), NULL) < 0)) + return false;
The original is easier to read to me - the else branch is more visible than the exclamation mark.
}
vshTablePrintToStdout(table, ctl); - return true; }
@@ -808,16 +795,15 @@ cmdDomIfGetLink(vshControl *ctl, const vshCmd *cmd) return false; }
- if (ninterfaces < 1) { - if (macstr[0]) + if (ninterfaces != 1) { + if (ninterfaces > 1)
It's more readable to me to duplicate the 'return else' rather than the condition. In the spirit of shorter if's than elses [ https://libvirt.org/coding-style.html#curly-braces ] you can simply move the n > 1 condition above n < 1
+ vshError(ctl, _("multiple matching interfaces found")); + else if (macstr[0]) vshError(ctl, _("Interface (mac: %s) not found."), macstr); else vshError(ctl, _("Interface (dev: %s) not found."), iface);
return false; - } else if (ninterfaces > 1) { - vshError(ctl, _("multiple matching interfaces found")); - return false; }
ctxt->node = interfaces[0];
Jano
This patch includes: * fix of a mistake in cmdMigrateSetMaxDowntime() - function returned false when it should have returned true * use of an early return in case of an error * removal of unnecessary variables and extra labels * returning value directly Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 250 +++++++++++++++++-------------------------- 1 file changed, 98 insertions(+), 152 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f876f30cc5..3232463485 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; } @@ -659,12 +659,10 @@ 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)) @@ -2705,7 +2703,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"); @@ -2738,13 +2735,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); } /* @@ -2930,7 +2924,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; @@ -2949,12 +2942,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 @@ -3428,19 +3420,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; } /* @@ -5066,7 +5056,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++) { @@ -5081,14 +5070,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 @@ -5539,7 +5525,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")); @@ -5554,10 +5539,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 @@ -5696,7 +5679,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"); @@ -5737,10 +5719,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; } /* @@ -5825,20 +5806,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; } /* @@ -5912,13 +5891,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; } @@ -5988,13 +5967,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; } @@ -6020,20 +5998,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; } /* @@ -6479,15 +6455,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; } /* @@ -7042,8 +7017,7 @@ virshParseCPUList(vshControl *ctl, int *cpumaplen, } } - if (virBitmapToData(map, &cpumap, cpumaplen) < 0) - goto cleanup; + virBitmapToData(map, &cpumap, cpumaplen); cleanup: virBitmapFree(map); @@ -8212,7 +8186,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; @@ -8232,14 +8205,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; } /* @@ -8268,7 +8241,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; @@ -8284,14 +8256,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; } /* @@ -9990,7 +9961,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"); @@ -10010,14 +9980,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; } /* @@ -10051,7 +10018,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; @@ -10067,13 +10033,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; } /* @@ -11006,25 +10970,19 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; unsigned long long downtime = 0; - bool ret = false; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; if (vshCommandOptULongLong(ctl, cmd, "downtime", &downtime) < 0) - goto done; + return false; + if (downtime < 1) { vshError(ctl, "%s", _("migrate: Invalid downtime")); - goto done; + return false; } - if (virDomainMigrateSetMaxDowntime(dom, downtime, 0)) - goto done; - - ret = true; - - done: - return ret; + return virDomainMigrateSetMaxDowntime(dom, downtime, 0) == 0; } @@ -11100,12 +11058,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; @@ -11486,11 +11444,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) { @@ -11947,12 +11903,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); @@ -11960,13 +11916,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) { @@ -11989,7 +11945,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]; } @@ -11999,22 +11955,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; } @@ -13666,10 +13618,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; @@ -13679,16 +13631,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[] = { @@ -13714,10 +13663,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; @@ -13727,16 +13676,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
On a Thursday in 2021, Kristina Hanicova wrote:
This patch includes: * fix of a mistake in cmdMigrateSetMaxDowntime() - function returned false when it should have returned true
Ideally, refactors should not alter behavior, so fixes should be separated. Jano
* use of an early return in case of an error * removal of unnecessary variables and extra labels * returning value directly
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 250 +++++++++++++++++-------------------------- 1 file changed, 98 insertions(+), 152 deletions(-)
This patch changes the way functions return bool value from pattern: if (functionCall() < 0) return false; return true; into a more readable and modern way of direct return: return functionCall() == 0; I know that not everybody will agree this is more readable so I am open to discussion and I leave merging this patch up to the reviewer. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 114 ++++++++++--------------------------------- 1 file changed, 26 insertions(+), 88 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3232463485..ec427443c4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -765,7 +765,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) return false; } - vshPrintExtra(ctl, "%s", _("Disk attached successfully\n")); return true; } @@ -1432,7 +1431,6 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) goto save_error; } - if (nparams == 0) { if (virDomainGetBlockIoTune(dom, NULL, NULL, &nparams, flags) != 0) { vshError(ctl, "%s", @@ -2673,10 +2671,7 @@ virshBlockJobSetSpeed(vshControl *ctl, if (vshBlockJobOptionBandwidth(ctl, cmd, bytes, &bandwidth) < 0) return false; - if (virDomainBlockJobSetSpeed(dom, path, bandwidth, flags) < 0) - return false; - - return true; + return virDomainBlockJobSetSpeed(dom, path, bandwidth, flags) == 0; } @@ -2693,10 +2688,7 @@ virshBlockJobAbort(virDomainPtr dom, if (pivot) flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; - if (virDomainBlockJobAbort(dom, path, flags) < 0) - return false; - - return true; + return virDomainBlockJobAbort(dom, path, flags) == 0; } @@ -3010,10 +3002,8 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom, vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]); vshPrintExtra(ctl, "\n"); fflush(stdout); - if (virshRunConsole(ctl, dom, name, flags) == 0) - return true; - return false; + return virshRunConsole(ctl, dom, name, flags) == 0; } static bool @@ -7079,15 +7069,10 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) return false; /* use old API without any explicit flags */ - if (flags == VIR_DOMAIN_AFFECT_CURRENT && !current) { - if (virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) != 0) - return false; - } else { - if (virDomainPinVcpuFlags(dom, vcpu, cpumap, cpumaplen, flags) != 0) - return false; - } + if (flags == VIR_DOMAIN_AFFECT_CURRENT && !current) + return virDomainPinVcpu(dom, vcpu, cpumap, cpumaplen) == 0; - return true; + return virDomainPinVcpuFlags(dom, vcpu, cpumap, cpumaplen, flags) == 0; } /* @@ -7184,10 +7169,7 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd) if (flags == -1) flags = VIR_DOMAIN_AFFECT_LIVE; - if (virDomainPinEmulator(dom, cpumap, cpumaplen, flags) != 0) - return false; - - return true; + return virDomainPinEmulator(dom, cpumap, cpumaplen, flags) == 0; } /* @@ -7270,15 +7252,10 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) } /* none of the options were specified */ - if (!current && flags == 0) { - if (virDomainSetVcpus(dom, count) != 0) - return false; - } else { - if (virDomainSetVcpusFlags(dom, count, flags) < 0) - return false; - } + if (!current && flags == 0) + return virDomainSetVcpus(dom, count) == 0; - return true; + return virDomainSetVcpusFlags(dom, count, flags) == 0; } @@ -7439,10 +7416,7 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) if (enable) state = 1; - if (virDomainSetVcpu(dom, vcpulist, state, flags) < 0) - return false; - - return true; + return virDomainSetVcpu(dom, vcpulist, state, flags) == 0; } @@ -7493,10 +7467,7 @@ cmdDomblkthreshold(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - if (virDomainSetBlockThreshold(dom, dev, threshold, 0) < 0) - return false; - - return true; + return virDomainSetBlockThreshold(dom, dev, threshold, 0) == 0; } @@ -7656,11 +7627,8 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd) if (!(cpumap = virshParseCPUList(ctl, &cpumaplen, cpulist, maxcpu))) return false; - if (virDomainPinIOThread(dom, iothread_id, - cpumap, cpumaplen, flags) != 0) - return false; - - return true; + return virDomainPinIOThread(dom, iothread_id, cpumap, + cpumaplen, flags) == 0; } /* @@ -7717,10 +7685,7 @@ cmdIOThreadAdd(vshControl *ctl, const vshCmd *cmd) return false; } - if (virDomainAddIOThread(dom, iothread_id, flags) < 0) - return false; - - return true; + return virDomainAddIOThread(dom, iothread_id, flags) == 0; } @@ -7877,15 +7842,13 @@ cmdIOThreadDel(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptInt(ctl, cmd, "id", &iothread_id) < 0) return false; + if (iothread_id <= 0) { vshError(ctl, _("Invalid IOThread id value: '%d'"), iothread_id); return false; } - if (virDomainDelIOThread(dom, iothread_id, flags) < 0) - return false; - - return true; + return virDomainDelIOThread(dom, iothread_id, flags) == 0; } /* @@ -8593,10 +8556,7 @@ cmdInjectNMI(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - if (virDomainInjectNMI(dom, 0) < 0) - return false; - - return true; + return virDomainInjectNMI(dom, 0) == 0; } /* @@ -8693,10 +8653,7 @@ cmdSendKey(vshControl *ctl, const vshCmd *cmd) count++; } - if (virDomainSendKey(dom, codeset, holdtime, keycodes, count, 0) < 0) - return false; - - return true; + return virDomainSendKey(dom, codeset, holdtime, keycodes, count, 0) == 0; } /* @@ -8788,10 +8745,7 @@ cmdSendProcessSignal(vshControl *ctl, const vshCmd *cmd) return false; } - if (virDomainSendProcessSignal(dom, pid_value, signum, 0) < 0) - return false; - - return true; + return virDomainSendProcessSignal(dom, pid_value, signum, 0) == 0; } /* @@ -8862,10 +8816,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) return false; kibibytes = VIR_DIV_UP(bytes, 1024); - if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) - return false; - - return true; + return virDomainSetMemoryFlags(dom, kibibytes, flags) == 0; } /* @@ -11118,10 +11069,7 @@ cmdMigrateSetMaxSpeed(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "postcopy")) flags |= VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY; - if (virDomainMigrateSetMaxSpeed(dom, bandwidth, flags) < 0) - return false; - - return true; + return virDomainMigrateSetMaxSpeed(dom, bandwidth, flags) == 0; } /* @@ -11194,10 +11142,7 @@ cmdMigratePostCopy(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - if (virDomainMigrateStartPostCopy(dom, 0) < 0) - return false; - - return true; + return virDomainMigrateStartPostCopy(dom, 0) == 0; } /* @@ -13801,10 +13746,7 @@ cmdGuestAgentTimeout(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptInt(ctl, cmd, "timeout", &timeout) < 0) return false; - if (virDomainAgentSetResponseTimeout(dom, timeout, flags) < 0) - return false; - - return true; + return virDomainAgentSetResponseTimeout(dom, timeout, flags) == 0; } /* @@ -14032,12 +13974,8 @@ cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd) } } - if (virDomainAuthorizedSSHKeysSet(dom, user, - (const char **) keys, nkeys, flags) < 0) { - return false; - } - - return true; + return virDomainAuthorizedSSHKeysSet(dom, user, (const char **) keys, + nkeys, flags) == 0; } -- 2.31.1
On Thu, Sep 23, 2021 at 17:08:02 +0200, Kristina Hanicova wrote:
This patch changes the way functions return bool value from pattern:
if (functionCall() < 0) return false;
return true;
into a more readable and modern way of direct return:
return functionCall() == 0;
I know that not everybody will agree this is more readable so I am open to discussion and I leave merging this patch up to the reviewer.
I agree that this is a matter of taste. My main counter-argument is that in case you need to add more code after what was the last 'functionCall', you then need to undo the pattern you've added. So in effort to minimize churn IMO the existing approach is better.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 114 ++++++++++--------------------------------- 1 file changed, 26 insertions(+), 88 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3232463485..ec427443c4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -765,7 +765,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) return false; }
- vshPrintExtra(ctl, "%s", _("Disk attached successfully\n")); return true; } @@ -1432,7 +1431,6 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) goto save_error; }
-
You are mixing changes not related to what the commit message describes.
This patch refactors a few bigger functions mainly trying to remove too deep indentation and make them more readable and more consistent with the rest of the file. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 203 ++++++++++++++++++++----------------------- tools/virsh-host.c | 140 ++++++++++++++--------------- tools/virsh-volume.c | 56 ++++++------ 3 files changed, 190 insertions(+), 209 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ec427443c4..2881956d78 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5133,7 +5133,6 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) int nparams = 0; int nupdates = 0; size_t i; - int ret; bool ret_val = false; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; bool current = vshCommandOptBool(cmd, "current"); @@ -5161,64 +5160,61 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (nparams) { - params = g_new0(virTypedParameter, nparams); + if (!nparams) + goto cleanup; - memset(params, 0, sizeof(*params) * nparams); - if (flags || current) { - /* We cannot query both live and config at once, so settle - on current in that case. If we are setting, then the - two values should match when we re-query; otherwise, we - report the error later. */ - ret = virDomainGetSchedulerParametersFlags(dom, params, &nparams, - ((live && config) ? 0 - : flags)); - } else { - ret = virDomainGetSchedulerParameters(dom, params, &nparams); - } - if (ret == -1) + params = g_new0(virTypedParameter, nparams); + memset(params, 0, sizeof(*params) * nparams); + + if (flags || current) { + /* We cannot query both live and config at once, so settle + on current in that case. If we are setting, then the + two values should match when we re-query; otherwise, we + report the error later. */ + if (virDomainGetSchedulerParametersFlags(dom, params, &nparams, + ((live && config) ? 0 : flags)) == -1) goto cleanup; - - /* See if any params are being set */ - if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams, - &updates)) < 0) + } else { + if (virDomainGetSchedulerParameters(dom, params, &nparams) == -1) goto cleanup; + } - /* Update parameters & refresh data */ - if (nupdates > 0) { - if (flags || current) - ret = virDomainSetSchedulerParametersFlags(dom, updates, - nupdates, flags); - else - ret = virDomainSetSchedulerParameters(dom, updates, nupdates); + /* See if any params are being set */ + if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams, + &updates)) < 0) + goto cleanup; - if (ret == -1) + /* Update parameters & refresh data */ + if (nupdates > 0) { + if (flags || current) { + if (virDomainSetSchedulerParametersFlags(dom, updates, + nupdates, flags) == -1) goto cleanup; - if (flags || current) - ret = virDomainGetSchedulerParametersFlags(dom, params, - &nparams, - ((live && config) ? 0 - : flags)); - else - ret = virDomainGetSchedulerParameters(dom, params, &nparams); - if (ret == -1) + if (virDomainGetSchedulerParametersFlags(dom, params, &nparams, + ((live && config) ? 0 : flags)) == -1) goto cleanup; } else { - /* When not doing --set, --live and --config do not mix. */ - if (live && config) { - vshError(ctl, "%s", - _("cannot query both live and config at once")); + if (virDomainSetSchedulerParameters(dom, updates, nupdates) == -1) goto cleanup; - } - } - ret_val = true; - for (i = 0; i < nparams; i++) { - char *str = vshGetTypedParamValue(ctl, ¶ms[i]); - vshPrint(ctl, "%-15s: %s\n", params[i].field, str); - VIR_FREE(str); + if (virDomainGetSchedulerParameters(dom, params, &nparams) == -1) + goto cleanup; } + } else { + /* When not doing --set, --live and --config do not mix. */ + if (live && config) { + vshError(ctl, "%s", + _("cannot query both live and config at once")); + goto cleanup; + } + } + + ret_val = true; + for (i = 0; i < nparams; i++) { + char *str = vshGetTypedParamValue(ctl, ¶ms[i]); + vshPrint(ctl, "%-15s: %s\n", params[i].field, str); + VIR_FREE(str); } cleanup: @@ -6503,7 +6499,6 @@ virshCPUCountCollect(vshControl *ctl, unsigned int flags, bool checkState) { - int ret = -2; virDomainInfo info; int count; g_autoptr(xmlDoc) xml = NULL; @@ -6524,11 +6519,11 @@ virshCPUCountCollect(vshControl *ctl, /* fallback code */ if (!(last_error->code == VIR_ERR_NO_SUPPORT || last_error->code == VIR_ERR_INVALID_ARG)) - goto cleanup; + return -2; if (flags & VIR_DOMAIN_VCPU_GUEST) { vshError(ctl, "%s", _("Failed to retrieve vCPU count from the guest")); - goto cleanup; + return -2; } if (!(flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) && @@ -6538,36 +6533,32 @@ virshCPUCountCollect(vshControl *ctl, vshResetLibvirtError(); if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { - count = virDomainGetMaxVcpus(dom); - } else { - if (virDomainGetInfo(dom, &info) < 0) - goto cleanup; + if (flags & VIR_DOMAIN_VCPU_MAXIMUM) + return virDomainGetMaxVcpus(dom); - count = info.nrVirtCpu; + if (virDomainGetInfo(dom, &info) < 0) + return -2; + + return info.nrVirtCpu; + } + + if (virshDomainGetXMLFromDom(ctl, dom, VIR_DOMAIN_XML_INACTIVE, + &xml, &ctxt) < 0) + return -2; + + if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { + if (virXPathInt("string(/domain/vcpu)", ctxt, &count) < 0) { + vshError(ctl, "%s", _("Failed to retrieve maximum vcpu count")); + return -2; } } else { - if (virshDomainGetXMLFromDom(ctl, dom, VIR_DOMAIN_XML_INACTIVE, - &xml, &ctxt) < 0) - goto cleanup; - - if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { - if (virXPathInt("string(/domain/vcpu)", ctxt, &count) < 0) { - vshError(ctl, "%s", _("Failed to retrieve maximum vcpu count")); - goto cleanup; - } - } else { - if (virXPathInt("string(/domain/vcpu/@current)", ctxt, &count) < 0) { - vshError(ctl, "%s", _("Failed to retrieve current vcpu count")); - goto cleanup; - } + if (virXPathInt("string(/domain/vcpu/@current)", ctxt, &count) < 0) { + vshError(ctl, "%s", _("Failed to retrieve current vcpu count")); + return -2; } } - ret = count; - cleanup: - - return ret; + return count; } static bool @@ -9588,7 +9579,7 @@ cmdQemuMonitorEvent(vshControl *ctl, const vshCmd *cmd) vshEventCleanup(ctl); if (eventId >= 0 && virConnectDomainQemuMonitorEventDeregister(priv->conn, eventId) < 0) - ret = false; + return false; return ret; } @@ -9790,6 +9781,7 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) int nfdlist; int *fdlist; size_t i; + int status; bool setlabel = true; g_autofree virSecurityModelPtr secmodel = NULL; g_autofree virSecurityLabelPtr seclabel = NULL; @@ -9828,40 +9820,8 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) */ if ((pid = virFork()) < 0) return false; - if (pid == 0) { - int status; - - if (setlabel && - virDomainLxcEnterSecurityLabel(secmodel, - seclabel, - NULL, - 0) < 0) - _exit(EXIT_CANCELED); - - if (virDomainLxcEnterCGroup(dom, 0) < 0) - _exit(EXIT_CANCELED); - - if (virDomainLxcEnterNamespace(dom, - nfdlist, - fdlist, - NULL, - NULL, - 0) < 0) - _exit(EXIT_CANCELED); - - /* Fork a second time because entering the - * pid namespace only takes effect after fork - */ - if ((pid = virFork()) < 0) - _exit(EXIT_CANCELED); - if (pid == 0) { - execv(cmdargv[0], cmdargv); - _exit(errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE); - } - if (virProcessWait(pid, &status, true) < 0) - _exit(EXIT_CANNOT_INVOKE); - virProcessExitWithStatus(status); - } else { + + if (pid != 0) { for (i = 0; i < nfdlist; i++) VIR_FORCE_CLOSE(fdlist[i]); VIR_FREE(fdlist); @@ -9869,8 +9829,33 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) vshReportError(ctl); return false; } + return true; + } + + if (setlabel && + virDomainLxcEnterSecurityLabel(secmodel, seclabel, NULL, 0) < 0) + _exit(EXIT_CANCELED); + + if (virDomainLxcEnterCGroup(dom, 0) < 0) + _exit(EXIT_CANCELED); + + if (virDomainLxcEnterNamespace(dom, nfdlist, fdlist, NULL, NULL, 0) < 0) + _exit(EXIT_CANCELED); + + /* Fork a second time because entering the + * pid namespace only takes effect after fork + */ + if ((pid = virFork()) < 0) + _exit(EXIT_CANCELED); + + if (pid == 0) { + execv(cmdargv[0], cmdargv); + _exit(errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE); } + if (virProcessWait(pid, &status, true) < 0) + _exit(EXIT_CANNOT_INVOKE); + virProcessExitWithStatus(status); return true; } diff --git a/tools/virsh-host.c b/tools/virsh-host.c index e6ed4a26ce..591746655b 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -162,7 +162,6 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) bool cellno = vshCommandOptBool(cmd, "cellno"); size_t i; g_autofree char *cap_xml = NULL; - g_autoptr(xmlDoc) xml = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; virshControl *priv = ctl->privData; @@ -171,68 +170,69 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) if (cellno && vshCommandOptInt(ctl, cmd, "cellno", &cell) < 0) return false; - if (all) { - if (!(cap_xml = virConnectGetCapabilities(priv->conn))) { - vshError(ctl, "%s", _("unable to get node capabilities")); - return false; - } + if (!all) { + if (cellno) { + if (virNodeGetCellsFreeMemory(priv->conn, &memory, cell, 1) != 1) + return false; - xml = virXMLParseStringCtxt(cap_xml, _("(capabilities)"), &ctxt); - if (!xml) { - vshError(ctl, "%s", _("unable to get node capabilities")); - return false; + vshPrint(ctl, "%d: %llu KiB\n", cell, (memory/1024)); + return true; } - nodes_cnt = virXPathNodeSet("/capabilities/host/topology/cells/cell", - ctxt, &nodes); - - if (nodes_cnt == -1) { - vshError(ctl, "%s", _("could not get information about " - "NUMA topology")); + if ((memory = virNodeGetFreeMemory(priv->conn)) == 0) return false; - } - nodes_free = g_new0(unsigned long long, nodes_cnt); - nodes_id = g_new0(unsigned long, nodes_cnt); + vshPrint(ctl, "%s: %llu KiB\n", _("Total"), (memory/1024)); + return true; + } - for (i = 0; i < nodes_cnt; i++) { - unsigned long id; - g_autofree char *val = virXMLPropString(nodes[i], "id"); - if (virStrToLong_ulp(val, NULL, 10, &id)) { - vshError(ctl, "%s", _("conversion from string failed")); - return false; - } - nodes_id[i] = id; - if (virNodeGetCellsFreeMemory(priv->conn, &(nodes_free[i]), - id, 1) != 1) { - vshError(ctl, _("failed to get free memory for NUMA node " - "number: %lu"), id); - return false; - } - } + if (!(cap_xml = virConnectGetCapabilities(priv->conn))) { + vshError(ctl, "%s", _("unable to get node capabilities")); + return false; + } - for (cell = 0; cell < nodes_cnt; cell++) { - vshPrint(ctl, "%5lu: %10llu KiB\n", nodes_id[cell], - (nodes_free[cell]/1024)); - memory += nodes_free[cell]; - } + if (!virXMLParseStringCtxt(cap_xml, _("(capabilities)"), &ctxt)) { + vshError(ctl, "%s", _("unable to get node capabilities")); + return false; + } - vshPrintExtra(ctl, "--------------------\n"); - vshPrintExtra(ctl, "%5s: %10llu KiB\n", _("Total"), memory/1024); - } else { - if (cellno) { - if (virNodeGetCellsFreeMemory(priv->conn, &memory, cell, 1) != 1) - return false; + nodes_cnt = virXPathNodeSet("/capabilities/host/topology/cells/cell", + ctxt, &nodes); - vshPrint(ctl, "%d: %llu KiB\n", cell, (memory/1024)); - } else { - if ((memory = virNodeGetFreeMemory(priv->conn)) == 0) - return false; + if (nodes_cnt == -1) { + vshError(ctl, "%s", _("could not get information about " + "NUMA topology")); + return false; + } + + nodes_free = g_new0(unsigned long long, nodes_cnt); + nodes_id = g_new0(unsigned long, nodes_cnt); - vshPrint(ctl, "%s: %llu KiB\n", _("Total"), (memory/1024)); + for (i = 0; i < nodes_cnt; i++) { + unsigned long id; + g_autofree char *val = virXMLPropString(nodes[i], "id"); + if (virStrToLong_ulp(val, NULL, 10, &id)) { + vshError(ctl, "%s", _("conversion from string failed")); + return false; + } + nodes_id[i] = id; + if (virNodeGetCellsFreeMemory(priv->conn, &(nodes_free[i]), + id, 1) != 1) { + vshError(ctl, _("failed to get free memory for NUMA node " + "number: %lu"), id); + return false; } } + for (cell = 0; cell < nodes_cnt; cell++) { + vshPrint(ctl, "%5lu: %10llu KiB\n", nodes_id[cell], + (nodes_free[cell]/1024)); + memory += nodes_free[cell]; + } + + vshPrintExtra(ctl, "--------------------\n"); + vshPrintExtra(ctl, "%5s: %10llu KiB\n", _("Total"), memory/1024); + return true; } @@ -804,30 +804,30 @@ cmdNodeCpuStats(vshControl *ctl, const vshCmd *cmd) cpu_stats[i]); } } + return true; + } + + if (present[VIRSH_CPU_USAGE]) { + vshPrint(ctl, "%-15s %5.1llu%%\n", + _("usage:"), cpu_stats[VIRSH_CPU_USAGE]); + vshPrint(ctl, "%-15s %5.1llu%%\n", + _("idle:"), 100 - cpu_stats[VIRSH_CPU_USAGE]); } else { - if (present[VIRSH_CPU_USAGE]) { - vshPrint(ctl, "%-15s %5.1llu%%\n", - _("usage:"), cpu_stats[VIRSH_CPU_USAGE]); - vshPrint(ctl, "%-15s %5.1llu%%\n", - _("idle:"), 100 - cpu_stats[VIRSH_CPU_USAGE]); - } else { - double usage, total_time = 0; - for (i = 0; i < VIRSH_CPU_USAGE; i++) - total_time += cpu_stats[i]; - - usage = (cpu_stats[VIRSH_CPU_USER] + cpu_stats[VIRSH_CPU_SYSTEM]) - / total_time * 100; - - vshPrint(ctl, "%-15s %5.1lf%%\n", _("usage:"), usage); - for (i = 0; i < VIRSH_CPU_USAGE; i++) { - if (present[i]) { - vshPrint(ctl, "%-15s %5.1lf%%\n", _(virshCPUOutput[i]), - cpu_stats[i] / total_time * 100); - } + double usage, total_time = 0; + for (i = 0; i < VIRSH_CPU_USAGE; i++) + total_time += cpu_stats[i]; + + usage = (cpu_stats[VIRSH_CPU_USER] + cpu_stats[VIRSH_CPU_SYSTEM]) + / total_time * 100; + + vshPrint(ctl, "%-15s %5.1lf%%\n", _("usage:"), usage); + for (i = 0; i < VIRSH_CPU_USAGE; i++) { + if (present[i]) { + vshPrint(ctl, "%-15s %5.1lf%%\n", _(virshCPUOutput[i]), + cpu_stats[i] / total_time * 100); } } } - return true; } diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 152f5b0dbe..ef437413df 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -1057,7 +1057,6 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) virStorageVolPtr vol; bool bytes = vshCommandOptBool(cmd, "bytes"); bool physical = vshCommandOptBool(cmd, "physical"); - bool ret = true; int rc; unsigned int flags = 0; @@ -1074,41 +1073,39 @@ cmdVolInfo(vshControl *ctl, const vshCmd *cmd) else rc = virStorageVolGetInfo(vol, &info); - if (rc == 0) { - double val; - const char *unit; + if (rc < 0) { + virStorageVolFree(vol); + return false; + } - vshPrint(ctl, "%-15s %s\n", _("Type:"), - virshVolumeTypeToString(info.type)); + vshPrint(ctl, "%-15s %s\n", _("Type:"), + virshVolumeTypeToString(info.type)); - if (bytes) { - vshPrint(ctl, "%-15s %llu %s\n", _("Capacity:"), - info.capacity, _("bytes")); - } else { - val = vshPrettyCapacity(info.capacity, &unit); - vshPrint(ctl, "%-15s %2.2lf %s\n", _("Capacity:"), val, unit); - } + if (bytes) { + vshPrint(ctl, "%-15s %llu %s\n", _("Capacity:"), + info.capacity, _("bytes")); - if (bytes) { - if (physical) - vshPrint(ctl, "%-15s %llu %s\n", _("Physical:"), - info.allocation, _("bytes")); - else - vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"), - info.allocation, _("bytes")); - } else { - val = vshPrettyCapacity(info.allocation, &unit); - if (physical) - vshPrint(ctl, "%-15s %2.2lf %s\n", _("Physical:"), val, unit); - else - vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val, unit); - } + if (physical) + vshPrint(ctl, "%-15s %llu %s\n", _("Physical:"), + info.allocation, _("bytes")); + else + vshPrint(ctl, "%-15s %llu %s\n", _("Allocation:"), + info.allocation, _("bytes")); } else { - ret = false; + const char *unit; + double val = vshPrettyCapacity(info.capacity, &unit); + + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Capacity:"), val, unit); + + val = vshPrettyCapacity(info.allocation, &unit); + if (physical) + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Physical:"), val, unit); + else + vshPrint(ctl, "%-15s %2.2lf %s\n", _("Allocation:"), val, unit); } virStorageVolFree(vol); - return ret; + return true; } /* @@ -1203,7 +1200,6 @@ cmdVolResize(vshControl *ctl, const vshCmd *cmd) delta ? _("Failed to change size of volume '%s' by %s") : _("Failed to change size of volume '%s' to %s"), virStorageVolGetName(vol), capacityStr); - ret = false; } cleanup: -- 2.31.1
This patch targets smaller functions and rewrites them into the pattern using an early return without needing redundant 'ret' variable (if possible). Patch also includes removal of 'else' branch after 'return' and other small alterations. Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-host.c | 26 ++++++----------- tools/virsh-interface.c | 65 ++++++++++++++++++----------------------- tools/virsh-network.c | 47 +++++++++++++---------------- tools/virsh-nodedev.c | 32 ++++++++------------ tools/virsh-nwfilter.c | 15 +++++----- tools/virsh-util.c | 3 +- 6 files changed, 77 insertions(+), 111 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 591746655b..66cd844263 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1243,7 +1243,6 @@ static bool cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) { const char *from = NULL; - bool ret = false; g_autofree char *result = NULL; g_auto(GStrv) list = NULL; unsigned int flags = 0; @@ -1260,16 +1259,13 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) if (!(list = vshExtractCPUDefXMLs(ctl, from))) return false; - result = virConnectBaselineCPU(priv->conn, (const char **)list, - g_strv_length(list), - flags); - - if (result) { - vshPrint(ctl, "%s", result); - ret = true; - } + if (!(result = virConnectBaselineCPU(priv->conn, (const char **)list, + g_strv_length(list), + flags))) + return false; - return ret; + vshPrint(ctl, "%s", result); + return true; } /* @@ -1355,7 +1351,6 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) unsigned long includeVersion; unsigned long apiVersion; unsigned long daemonVersion; - int ret; unsigned int major; unsigned int minor; unsigned int rel; @@ -1375,8 +1370,7 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) vshPrint(ctl, _("Compiled against library: libvirt %d.%d.%d\n"), major, minor, rel); - ret = virGetVersion(&libVersion, hvType, &apiVersion); - if (ret < 0) { + if (virGetVersion(&libVersion, hvType, &apiVersion) < 0) { vshError(ctl, "%s", _("failed to get the library version")); return false; } @@ -1394,8 +1388,7 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) vshPrint(ctl, _("Using API: %s %d.%d.%d\n"), hvType, major, minor, rel); - ret = virConnectGetVersion(priv->conn, &hvVersion); - if (ret < 0) { + if (virConnectGetVersion(priv->conn, &hvVersion) < 0) { vshError(ctl, "%s", _("failed to get the hypervisor version")); return false; } @@ -1413,8 +1406,7 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) } if (vshCommandOptBool(cmd, "daemon")) { - ret = virConnectGetLibVersion(priv->conn, &daemonVersion); - if (ret < 0) { + if (virConnectGetLibVersion(priv->conn, &daemonVersion) < 0) { vshError(ctl, "%s", _("failed to get the daemon version")); } else { major = daemonVersion / 1000000; diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 46af45c97b..f402119b68 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -485,26 +485,23 @@ static bool cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd) { virInterfacePtr iface; - bool ret = true; g_autofree char *dump = NULL; unsigned int flags = 0; - bool inactive = vshCommandOptBool(cmd, "inactive"); - if (inactive) + if (vshCommandOptBool(cmd, "inactive")) flags |= VIR_INTERFACE_XML_INACTIVE; if (!(iface = virshCommandOptInterface(ctl, cmd, NULL))) return false; - dump = virInterfaceGetXMLDesc(iface, flags); - if (dump != NULL) { - vshPrint(ctl, "%s", dump); - } else { - ret = false; + if (!(dump = virInterfaceGetXMLDesc(iface, flags))) { + virInterfaceFree(iface); + return false; } + vshPrint(ctl, "%s", dump); virInterfaceFree(iface); - return ret; + return true; } /* @@ -535,7 +532,6 @@ cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd) { virInterfacePtr iface; const char *from = NULL; - bool ret = true; g_autofree char *buffer = NULL; unsigned int flags = 0; virshControl *priv = ctl->privData; @@ -549,17 +545,15 @@ cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd) if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) return false; - iface = virInterfaceDefineXML(priv->conn, buffer, flags); - - if (iface != NULL) { - vshPrintExtra(ctl, _("Interface %s defined from %s\n"), - virInterfaceGetName(iface), from); - virInterfaceFree(iface); - } else { + if (!(iface = virInterfaceDefineXML(priv->conn, buffer, flags))) { vshError(ctl, _("Failed to define interface from %s"), from); - ret = false; + return false; } - return ret; + + vshPrintExtra(ctl, _("Interface %s defined from %s\n"), + virInterfaceGetName(iface), from); + virInterfaceFree(iface); + return true; } /* @@ -584,21 +578,20 @@ static bool cmdInterfaceUndefine(vshControl *ctl, const vshCmd *cmd) { virInterfacePtr iface; - bool ret = true; const char *name; if (!(iface = virshCommandOptInterface(ctl, cmd, &name))) return false; - if (virInterfaceUndefine(iface) == 0) { - vshPrintExtra(ctl, _("Interface %s undefined\n"), name); - } else { + if (virInterfaceUndefine(iface) < 0) { vshError(ctl, _("Failed to undefine interface %s"), name); - ret = false; + virInterfaceFree(iface); + return false; } + vshPrintExtra(ctl, _("Interface %s undefined\n"), name); virInterfaceFree(iface); - return ret; + return true; } /* @@ -623,21 +616,20 @@ static bool cmdInterfaceStart(vshControl *ctl, const vshCmd *cmd) { virInterfacePtr iface; - bool ret = true; const char *name; if (!(iface = virshCommandOptInterface(ctl, cmd, &name))) return false; - if (virInterfaceCreate(iface, 0) == 0) { - vshPrintExtra(ctl, _("Interface %s started\n"), name); - } else { + if (virInterfaceCreate(iface, 0) < 0) { vshError(ctl, _("Failed to start interface %s"), name); - ret = false; + virInterfaceFree(iface); + return false; } + vshPrintExtra(ctl, _("Interface %s started\n"), name); virInterfaceFree(iface); - return ret; + return true; } /* @@ -662,21 +654,20 @@ static bool cmdInterfaceDestroy(vshControl *ctl, const vshCmd *cmd) { virInterfacePtr iface; - bool ret = true; const char *name; if (!(iface = virshCommandOptInterface(ctl, cmd, &name))) return false; - if (virInterfaceDestroy(iface, 0) == 0) { - vshPrintExtra(ctl, _("Interface %s destroyed\n"), name); - } else { + if (virInterfaceDestroy(iface, 0) < 0) { vshError(ctl, _("Failed to destroy interface %s"), name); - ret = false; + virInterfaceFree(iface); + return false; } + vshPrintExtra(ctl, _("Interface %s destroyed\n"), name); virInterfaceFree(iface); - return ret; + return false; } /* diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 8651265909..1442210278 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -209,7 +209,6 @@ cmdNetworkCreate(vshControl *ctl, const vshCmd *cmd) { virNetworkPtr network; const char *from = NULL; - bool ret = true; g_autofree char *buffer = NULL; unsigned int flags = 0; virshControl *priv = ctl->privData; @@ -228,15 +227,15 @@ cmdNetworkCreate(vshControl *ctl, const vshCmd *cmd) else network = virNetworkCreateXML(priv->conn, buffer); - if (network != NULL) { - vshPrintExtra(ctl, _("Network %s created from %s\n"), - virNetworkGetName(network), from); - virNetworkFree(network); - } else { + if (!network) { vshError(ctl, _("Failed to create network from %s"), from); - ret = false; + return false; } - return ret; + + vshPrintExtra(ctl, _("Network %s created from %s\n"), + virNetworkGetName(network), from); + virNetworkFree(network); + return true; } /* @@ -267,7 +266,6 @@ cmdNetworkDefine(vshControl *ctl, const vshCmd *cmd) { virNetworkPtr network; const char *from = NULL; - bool ret = true; g_autofree char *buffer = NULL; unsigned int flags = 0; virshControl *priv = ctl->privData; @@ -286,15 +284,15 @@ cmdNetworkDefine(vshControl *ctl, const vshCmd *cmd) else network = virNetworkDefineXML(priv->conn, buffer); - if (network != NULL) { - vshPrintExtra(ctl, _("Network %s defined from %s\n"), - virNetworkGetName(network), from); - virNetworkFree(network); - } else { + if (!network) { vshError(ctl, _("Failed to define network from %s"), from); - ret = false; + return false; } - return ret; + + vshPrintExtra(ctl, _("Network %s defined from %s\n"), + virNetworkGetName(network), from); + virNetworkFree(network); + return true; } /* @@ -362,28 +360,23 @@ static bool cmdNetworkDumpXML(vshControl *ctl, const vshCmd *cmd) { virNetworkPtr network; - bool ret = true; g_autofree char *dump = NULL; unsigned int flags = 0; - int inactive; if (!(network = virshCommandOptNetwork(ctl, cmd, NULL))) return false; - inactive = vshCommandOptBool(cmd, "inactive"); - if (inactive) + if (vshCommandOptBool(cmd, "inactive")) flags |= VIR_NETWORK_XML_INACTIVE; - dump = virNetworkGetXMLDesc(network, flags); - - if (dump != NULL) { - vshPrint(ctl, "%s", dump); - } else { - ret = false; + if (!(dump = virNetworkGetXMLDesc(network, flags))) { + virNetworkFree(network); + return false; } + vshPrint(ctl, "%s", dump); virNetworkFree(network); - return ret; + return true; } /* diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index f1b4eb94bf..f72359121f 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -57,7 +57,6 @@ cmdNodeDeviceCreate(vshControl *ctl, const vshCmd *cmd) { virNodeDevicePtr dev = NULL; const char *from = NULL; - bool ret = true; g_autofree char *buffer = NULL; virshControl *priv = ctl->privData; @@ -67,18 +66,15 @@ cmdNodeDeviceCreate(vshControl *ctl, const vshCmd *cmd) if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) return false; - dev = virNodeDeviceCreateXML(priv->conn, buffer, 0); - - if (dev != NULL) { - vshPrintExtra(ctl, _("Node device %s created from %s\n"), - virNodeDeviceGetName(dev), from); - virNodeDeviceFree(dev); - } else { + if (!(dev = virNodeDeviceCreateXML(priv->conn, buffer, 0))) { vshError(ctl, _("Failed to create node device from %s"), from); - ret = false; + return false; } - return ret; + vshPrintExtra(ctl, _("Node device %s created from %s\n"), + virNodeDeviceGetName(dev), from); + virNodeDeviceFree(dev); + return true; } @@ -1077,7 +1073,6 @@ cmdNodeDeviceDefine(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) { virNodeDevice *dev = NULL; const char *from = NULL; - bool ret = true; g_autofree char *buffer = NULL; virshControl *priv = ctl->privData; @@ -1087,18 +1082,15 @@ cmdNodeDeviceDefine(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) return false; - dev = virNodeDeviceDefineXML(priv->conn, buffer, 0); - - if (dev != NULL) { - vshPrintExtra(ctl, _("Node device '%s' defined from '%s'\n"), - virNodeDeviceGetName(dev), from); - virNodeDeviceFree(dev); - } else { + if (!(dev = virNodeDeviceDefineXML(priv->conn, buffer, 0))) { vshError(ctl, _("Failed to define node device from '%s'"), from); - ret = false; + return false; } - return ret; + vshPrintExtra(ctl, _("Node device '%s' defined from '%s'\n"), + virNodeDeviceGetName(dev), from); + virNodeDeviceFree(dev); + return true; } diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c index 77f211d031..33164f623f 100644 --- a/tools/virsh-nwfilter.c +++ b/tools/virsh-nwfilter.c @@ -515,7 +515,6 @@ cmdNWFilterBindingCreate(vshControl *ctl, const vshCmd *cmd) { virNWFilterBindingPtr binding; const char *from = NULL; - bool ret = true; char *buffer; unsigned int flags = 0; virshControl *priv = ctl->privData; @@ -532,15 +531,15 @@ cmdNWFilterBindingCreate(vshControl *ctl, const vshCmd *cmd) binding = virNWFilterBindingCreateXML(priv->conn, buffer, flags); VIR_FREE(buffer); - if (binding != NULL) { - vshPrintExtra(ctl, _("Network filter binding on %s created from %s\n"), - virNWFilterBindingGetPortDev(binding), from); - virNWFilterBindingFree(binding); - } else { + if (!binding) { vshError(ctl, _("Failed to create network filter from %s"), from); - ret = false; + return false; } - return ret; + + vshPrintExtra(ctl, _("Network filter binding on %s created from %s\n"), + virNWFilterBindingGetPortDev(binding), from); + virNWFilterBindingFree(binding); + return true; } diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 19cd0bcb99..fb74514b3c 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -137,8 +137,7 @@ virshDomainState(vshControl *ctl, /* fall back to virDomainGetInfo if virDomainGetState is not supported */ if (virDomainGetInfo(dom, &info) < 0) return -1; - else - return info.state; + return info.state; } -- 2.31.1
On a Thursday in 2021, Kristina Hanicova wrote:
This patch targets smaller functions and rewrites them into the pattern using an early return without needing redundant 'ret' variable (if possible). Patch also includes removal of 'else' branch after 'return' and other small alterations.
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-host.c | 26 ++++++----------- tools/virsh-interface.c | 65 ++++++++++++++++++----------------------- tools/virsh-network.c | 47 +++++++++++++---------------- tools/virsh-nodedev.c | 32 ++++++++------------ tools/virsh-nwfilter.c | 15 +++++----- tools/virsh-util.c | 3 +- 6 files changed, 77 insertions(+), 111 deletions(-)
diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 591746655b..66cd844263 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1243,7 +1243,6 @@ static bool cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) { const char *from = NULL; - bool ret = false; g_autofree char *result = NULL; g_auto(GStrv) list = NULL; unsigned int flags = 0; @@ -1260,16 +1259,13 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) if (!(list = vshExtractCPUDefXMLs(ctl, from))) return false;
- result = virConnectBaselineCPU(priv->conn, (const char **)list, - g_strv_length(list), - flags); - - if (result) { - vshPrint(ctl, "%s", result); - ret = true; - } + if (!(result = virConnectBaselineCPU(priv->conn, (const char **)list, + g_strv_length(list), + flags))) + return false;
- return ret; + vshPrint(ctl, "%s", result);
Please put an emtpy line before the successful returns.
+ return true; }
/* diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 46af45c97b..f402119b68 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -485,26 +485,23 @@ static bool cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd) { virInterfacePtr iface; - bool ret = true; g_autofree char *dump = NULL; unsigned int flags = 0; - bool inactive = vshCommandOptBool(cmd, "inactive");
- if (inactive) + if (vshCommandOptBool(cmd, "inactive")) flags |= VIR_INTERFACE_XML_INACTIVE;
if (!(iface = virshCommandOptInterface(ctl, cmd, NULL))) return false;
- dump = virInterfaceGetXMLDesc(iface, flags); - if (dump != NULL) { - vshPrint(ctl, "%s", dump); - } else { - ret = false; + if (!(dump = virInterfaceGetXMLDesc(iface, flags))) { + virInterfaceFree(iface);
Rather than duplicating virInterfaceFree, you can create a virshInterface type and define a cleanup function for it, as we've done in tools/virsh-util.h for some other public structs.
+ return false; }
+ vshPrint(ctl, "%s", dump); virInterfaceFree(iface); - return ret; + return true; }
/*
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Kristina Hanicova -
Peter Krempa