10:17 a.m.
This is partially v2 of:
https://listman.redhat.com/archives/libvir-list/2021-September/msg00728.html
Diff to v1:
* split changes into smaller patches
Kristina Hanicova (7):
virsh: domain: use early return in virshDomainDefine()
virsh: domain: remove nested 'if' in cmdAttachDisk()
virsh: domain: remove 'ret' variable and use direct return when
possible
virsh: domain: remove 'ret' variable, use early return when possible
virsh: domain: use early return when possible
virsh: domain: remove 'ret' variable and use 'count' instead
virsh: domain: remove else branch
tools/virsh-domain.c | 232 +++++++++++++++++--------------------------
1 file changed, 92 insertions(+), 140 deletions(-)
--
2.31.1
10:17 a.m.
New subject: [PATCH v2 1/7] virsh: domain: use early return in virshDomainDefine()
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
tools/virsh-domain.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index bee3346eb0..edee548d8a 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;
}
--
2.31.1
10:17 a.m.
New subject: [PATCH v2 2/7] virsh: domain: remove nested 'if' in cmdAttachDisk()
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
tools/virsh-domain.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index edee548d8a..0d482f9e15 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -659,12 +659,9 @@ 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))
--
2.31.1
10:17 a.m.
New subject: [PATCH v2 3/7] virsh: domain: remove 'ret' variable and use direct return when possible
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
tools/virsh-domain.c | 62 ++++++++++++++++----------------------------
1 file changed, 22 insertions(+), 40 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0d482f9e15..1da9b183bf 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2702,7 +2702,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");
@@ -2735,13 +2734,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);
}
/*
@@ -5063,7 +5059,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++) {
@@ -5078,14 +5073,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
@@ -5529,7 +5521,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"));
@@ -5544,10 +5535,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
@@ -5686,7 +5675,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");
@@ -5727,10 +5715,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;
}
/*
@@ -6469,15 +6456,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;
}
/*
@@ -11926,12 +11912,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);
@@ -11939,13 +11925,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) {
@@ -11968,7 +11954,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];
}
@@ -11978,22 +11964,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;
}
--
2.31.1
10:17 a.m.
New subject: [PATCH v2 4/7] virsh: domain: remove 'ret' variable, use early return when possible
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
tools/virsh-domain.c | 84 +++++++++++++++++---------------------------
1 file changed, 33 insertions(+), 51 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1da9b183bf..1692197d1a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2923,7 +2923,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;
@@ -2942,12 +2941,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
@@ -3421,19 +3419,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;
}
/*
@@ -5802,20 +5798,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;
}
/*
@@ -5997,20 +5991,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;
}
/*
@@ -8183,7 +8175,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;
@@ -8203,14 +8194,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;
}
/*
@@ -8239,7 +8230,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;
@@ -8255,14 +8245,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;
}
/*
@@ -9955,7 +9944,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");
@@ -9975,14 +9963,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;
}
/*
@@ -10016,7 +10001,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;
@@ -10032,13 +10016,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;
}
/*
--
2.31.1
10:17 a.m.
New subject: [PATCH v2 5/7] virsh: domain: use early return when possible
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
tools/virsh-domain.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1692197d1a..3c496d845a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -5883,13 +5883,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;
}
@@ -5959,13 +5959,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;
}
--
2.31.1
10:17 a.m.
New subject: [PATCH v2 6/7] virsh: domain: remove 'ret' variable and use 'count' instead
This patch also includes use of an early return in case of an
error. I think the changes make the functions more readable.
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
tools/virsh-domain.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 3c496d845a..94ed786751 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -13608,10 +13608,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;
@@ -13621,16 +13621,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[] = {
@@ -13656,10 +13653,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;
@@ -13669,16 +13666,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
10:17 a.m.
New subject: [PATCH v2 7/7] virsh: domain: remove else branch
I removed else branches after return/break as they are not
necessary and the code looks cleaner without them.
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
tools/virsh-domain.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 94ed786751..ca8f08d949 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11046,12 +11046,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;
@@ -11432,11 +11432,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) {
--
2.31.1
3:06 a.m.
On 9/24/21 5:17 PM, Kristina Hanicova wrote:
This is partially v2 of:
https://listman.redhat.com/archives/libvir-list/2021-September/msg00728.html
Diff to v1:
* split changes into smaller patches
Kristina Hanicova (7):
virsh: domain: use early return in virshDomainDefine()
virsh: domain: remove nested 'if' in cmdAttachDisk()
virsh: domain: remove 'ret' variable and use direct return when
possible
virsh: domain: remove 'ret' variable, use early return when possible
virsh: domain: use early return when possible
virsh: domain: remove 'ret' variable and use 'count' instead
virsh: domain: remove else branch
tools/virsh-domain.c | 232 +++++++++++++++++--------------------------
1 file changed, 92 insertions(+), 140 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1154
days inactive
1155
days old
8 comments
2 participants
participants (2)
-
Kristina Hanicova -
Michal Prívozník