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(a)redhat.com&gt; --- 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 a Thursday in 2021, Kristina Hanicova wrote: 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. I was asked to leave an empty line before the final return by a reviewer recently, so it seems some people like it. You can return g_strdup("") directly. Another alternative could be to split the fallback code below into a separate function and call it if errCode == NO_SUPPORT The changes in cmdDomblkinfo deserve their own patch. [...] I prefer the version where we process the command options first and initialize the bool separately. The original is easier to read to me - the else branch is more visible than the exclamation mark. 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 Jano

On Thu, Sep 23, 2021 at 17:08:02 +0200, Kristina Hanicova wrote: 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. You are mixing changes not related to what the commit message describes.

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(a)redhat.com&gt; --- 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