6:49 p.m.
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
6:49 p.m.
New subject: [PATCH v2 1/4] virsh: remove variable 'ret' and use early return if possible
Signed-off-by: Kristina Hanicova <khanicov(a)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
6:49 p.m.
New subject: [PATCH v2 2/4] virsh: remove variable 'ret' in cmdVersion()
Signed-off-by: Kristina Hanicova <khanicov(a)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
6:49 p.m.
New subject: [PATCH v2 3/4] virsh: remove variable 'ret' and 'inactive'
Signed-off-by: Kristina Hanicova <khanicov(a)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
3:02 a.m.
New subject: [PATCH v2 3/4] virsh: remove variable 'ret' and 'inactive'
On 9/24/21 1:49 AM, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov(a)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
3:08 a.m.
New subject: [PATCH v2 3/4] virsh: remove variable 'ret' and 'inactive'
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(a)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
6:49 p.m.
New subject: [PATCH v2 4/4] virsh: util: remove 'else' branch after return
Signed-off-by: Kristina Hanicova <khanicov(a)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
3:02 a.m.
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(a)redhat.com>
Michal
1155
days inactive
1156
days old
7 comments
3 participants
participants (3)
-
Kristina Hanicova -
Michal Prívozník -
Peter Krempa