[PATCH 1/5] virsh: checkpoint & domain-monitor: small refactoring

[Ján Tomko]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210923/d186f5da/attachment-0001.sig>