[PATCH v2 0/4] virsh: refactor unnecessary variables and else branch
This is partially v2 of: https://listman.redhat.com/archives/libvir-list/2021-September/msg00731.html Diff to v1: * split changes into smaller patches Kristina Hanicova (4): virsh: remove variable 'ret' and use early return if possible virsh: remove variable 'ret' in cmdVersion() virsh: remove variable 'ret' and 'inactive' virsh: util: remove 'else' branch after return tools/virsh-host.c | 25 +++++----------- 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, 76 insertions(+), 111 deletions(-) -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-host.c | 15 +++++-------- tools/virsh-interface.c | 50 ++++++++++++++++++----------------------- tools/virsh-network.c | 30 ++++++++++++------------- tools/virsh-nodedev.c | 32 ++++++++++---------------- tools/virsh-nwfilter.c | 15 ++++++------- 5 files changed, 60 insertions(+), 82 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index e6ed4a26ce..b602696704 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,12 @@ 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; } /* diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 46af45c97b..4bcc59b580 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -535,7 +535,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 +548,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 +581,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 +619,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 +657,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..198993ac33 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; } /* 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; } -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-host.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index b602696704..6f306fffe3 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1350,7 +1350,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; @@ -1370,8 +1369,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; } @@ -1389,8 +1387,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; } @@ -1408,8 +1405,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; -- 2.31.1
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-interface.c | 15 ++++++--------- tools/virsh-network.c | 17 ++++++----------- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 4bcc59b580..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; } /* diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 198993ac33..1442210278 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -360,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; } /* -- 2.31.1
On 9/24/21 1:49 AM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-interface.c | 15 ++++++--------- tools/virsh-network.c | 17 ++++++----------- 2 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 4bcc59b580..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;
1: this ^^^
- 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;
I wonder whether we can declare an g_autoptr() cleanup for these public APIs. We can't do that in public headers BUT we could do it in something private, that's widely available (internal.h perhaps?). That way we could turn [1] into g_autoptr() and drop both these free calls. Obviously, that's orthogonal to what you are doing here and as such must go into a standalone patchset. Just an idea I had. Michal
On Fri, Sep 24, 2021 at 10:02:07 +0200, Michal Prívozník wrote:
On 9/24/21 1:49 AM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-interface.c | 15 ++++++--------- tools/virsh-network.c | 17 ++++++----------- 2 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 4bcc59b580..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;
1: this ^^^
+ vshPrint(ctl, "%s", dump); virInterfaceFree(iface); - return ret; + return true;
I wonder whether we can declare an g_autoptr() cleanup for these public APIs. We can't do that in public headers BUT we could do it in something private, that's widely available (internal.h perhaps?). That way we could turn [1] into g_autoptr() and drop both these free calls.
See 'virshDomain' and the associated cleanup registration in virsh-util.h Also https://listman.redhat.com/archives/libvir-list/2021-September/msg00751.html
Obviously, that's orthogonal to what you are doing here and as such must go into a standalone patchset. Just an idea I had.
Michal
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 9/24/21 1:49 AM, Kristina Hanicova wrote:
This is partially v2 of: https://listman.redhat.com/archives/libvir-list/2021-September/msg00731.html
Diff to v1: * split changes into smaller patches
Kristina Hanicova (4): virsh: remove variable 'ret' and use early return if possible virsh: remove variable 'ret' in cmdVersion() virsh: remove variable 'ret' and 'inactive' virsh: util: remove 'else' branch after return
tools/virsh-host.c | 25 +++++----------- 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, 76 insertions(+), 111 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Kristina Hanicova -
Michal Prívozník -
Peter Krempa