9:22 a.m.
This is all done in preparation for dropping needless labels such as:
cleanup:
return ret;
I'll post that patch as soon as these are merged.
Michal Prívozník (4):
virNetDevVPortProfileParse: Use g_autofree
examples/dommigrate: Don't set retval in usage()
examples/dommigrate: Make retval portable
virsh: Remove unnecessary else branches
examples/c/domain/dommigrate.c | 11 +++++-----
src/conf/netdev_vport_profile_conf.c | 30 +++++++++-----------------
tools/virsh-nodedev.c | 5 ++---
tools/virsh-volume.c | 32 +++++++++++++---------------
4 files changed, 32 insertions(+), 46 deletions(-)
--
2.32.0
9:22 a.m.
New subject: [PATCH 1/4] virNetDevVPortProfileParse: Use g_autofree
Explicit calls to VIR_FREE() can be dropped then.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/netdev_vport_profile_conf.c | 30 ++++++++++------------------
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/src/conf/netdev_vport_profile_conf.c b/src/conf/netdev_vport_profile_conf.c
index d98ce098df..dfffc4dd70 100644
--- a/src/conf/netdev_vport_profile_conf.c
+++ b/src/conf/netdev_vport_profile_conf.c
@@ -29,14 +29,14 @@
virNetDevVPortProfile *
virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags)
{
- char *virtPortType;
- char *virtPortManagerID = NULL;
- char *virtPortTypeID = NULL;
- char *virtPortTypeIDVersion = NULL;
- char *virtPortInstanceID = NULL;
- char *virtPortProfileID = NULL;
- char *virtPortInterfaceID = NULL;
- virNetDevVPortProfile *virtPort = NULL;
+ g_autofree char *virtPortType = NULL;
+ g_autofree char *virtPortManagerID = NULL;
+ g_autofree char *virtPortTypeID = NULL;
+ g_autofree char *virtPortTypeIDVersion = NULL;
+ g_autofree char *virtPortInstanceID = NULL;
+ g_autofree char *virtPortProfileID = NULL;
+ g_autofree char *virtPortInterfaceID = NULL;
+ g_autofree virNetDevVPortProfile *virtPort = NULL;
xmlNodePtr cur = node->children;
virtPort = g_new0(virNetDevVPortProfile, 1);
@@ -178,20 +178,10 @@ virNetDevVPortProfileParse(xmlNodePtr node, unsigned int flags)
if (virNetDevVPortProfileCheckNoExtras(virtPort) < 0)
goto error;
- cleanup:
- VIR_FREE(virtPortManagerID);
- VIR_FREE(virtPortTypeID);
- VIR_FREE(virtPortTypeIDVersion);
- VIR_FREE(virtPortInstanceID);
- VIR_FREE(virtPortProfileID);
- VIR_FREE(virtPortType);
- VIR_FREE(virtPortInterfaceID);
-
- return virtPort;
+ return g_steal_pointer(&virtPort);
error:
- VIR_FREE(virtPort);
- goto cleanup;
+ return NULL;
}
--
2.32.0
9:22 a.m.
New subject: [PATCH 2/4] examples/dommigrate: Don't set retval in usage()
The usage() function should just print expected arguments. Make
the function return void then.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
examples/c/domain/dommigrate.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/examples/c/domain/dommigrate.c b/examples/c/domain/dommigrate.c
index 60cfb3fb83..b1641efb9a 100644
--- a/examples/c/domain/dommigrate.c
+++ b/examples/c/domain/dommigrate.c
@@ -26,11 +26,10 @@
#include <libvirt/virterror.h>
-static int
-usage(char *prgn, int ret)
+static void
+usage(char *prgn)
{
printf("Usage: %s <src uri> <dst uri> <domain name>\n",
prgn);
- return ret;
}
int
@@ -42,7 +41,7 @@ main(int argc, char *argv[])
virDomainPtr dom = NULL;
if (argc < 4) {
- ret = usage(argv[0], 1);
+ usage(argv[0]);
goto out;
}
--
2.32.0
9:22 a.m.
New subject: [PATCH 3/4] examples/dommigrate: Make retval portable
Currently, the dommigrate example returns 0 or 1 for success or
failure state, respectively. Except for a few cases where it
forgot to change the @ret variable just before jumping onto the
'cleanup' label. Making the code follow our usual pattern
(initialize @ret to an error value and set it to success value
only at the end) fixes those cases. Also, using EXIT_SUCCESS and
EXIT_FAILURE is more portable (even though on my system they are
just an alias to values the example already uses).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
examples/c/domain/dommigrate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/examples/c/domain/dommigrate.c b/examples/c/domain/dommigrate.c
index b1641efb9a..3d32ada6d3 100644
--- a/examples/c/domain/dommigrate.c
+++ b/examples/c/domain/dommigrate.c
@@ -36,7 +36,7 @@ int
main(int argc, char *argv[])
{
char *src_uri, *dst_uri, *domname;
- int ret = 0;
+ int ret = EXIT_FAILURE;
virConnectPtr conn = NULL;
virDomainPtr dom = NULL;
@@ -52,7 +52,6 @@ main(int argc, char *argv[])
printf("Attempting to connect to the source hypervisor...\n");
conn = virConnectOpenAuth(src_uri, virConnectAuthPtrDefault, 0);
if (!conn) {
- ret = 1;
fprintf(stderr, "No connection to the source hypervisor: %s.\n",
virGetLastErrorMessage());
goto out;
@@ -74,6 +73,7 @@ main(int argc, char *argv[])
}
printf("Migration finished with success.\n");
+ ret = EXIT_SUCCESS;
cleanup:
if (dom != NULL)
--
2.32.0
9:22 a.m.
New subject: [PATCH 4/4] virsh: Remove unnecessary else branches
In a few cases we call a public API, wrapped in an if() statement
with both branches written out explicitly. The error branch jumps
onto cleanup label, while the successful prints out a message.
Right after these ifs there's 'ret = true;' and the cleanup
label. The code is a bit more readable if only the error branch
is kept and printing happens at the same level as setting the ret
variable.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virsh-nodedev.c | 5 ++---
tools/virsh-volume.c | 32 +++++++++++++++-----------------
2 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index 1ad8db7a3f..424865962f 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -1024,13 +1024,12 @@ cmdNodeDeviceUndefine(vshControl *ctl, const vshCmd *cmd
G_GNUC_UNUSED)
if (!dev)
goto cleanup;
- if (virNodeDeviceUndefine(dev, 0) == 0) {
- vshPrintExtra(ctl, _("Undefined node device '%s'\n"),
device_value);
- } else {
+ if (virNodeDeviceUndefine(dev, 0) < 0) {
vshError(ctl, _("Failed to undefine node device '%s'"),
device_value);
goto cleanup;
}
+ vshPrintExtra(ctl, _("Undefined node device '%s'\n"),
device_value);
ret = true;
cleanup:
return ret;
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 4c8e841701..12773d900b 100644
--- a/tools/virsh-volume.c
+++ b/tools/virsh-volume.c
@@ -411,14 +411,14 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
}
- if ((vol = virStorageVolCreateXML(pool, buffer, flags))) {
- vshPrintExtra(ctl, _("Vol %s created from %s\n"),
- virStorageVolGetName(vol), from);
- ret = true;
- } else {
+ if (!(vol = virStorageVolCreateXML(pool, buffer, flags))) {
vshError(ctl, _("Failed to create vol from %s"), from);
+ goto cleanup;
}
+ vshPrintExtra(ctl, _("Vol %s created from %s\n"),
+ virStorageVolGetName(vol), from);
+ ret = true;
cleanup:
return ret;
}
@@ -489,14 +489,13 @@ cmdVolCreateFrom(vshControl *ctl, const vshCmd *cmd)
newvol = virStorageVolCreateXMLFrom(pool, buffer, inputvol, flags);
- if (newvol != NULL) {
- vshPrintExtra(ctl, _("Vol %s created from input vol %s\n"),
- virStorageVolGetName(newvol), virStorageVolGetName(inputvol));
- } else {
+ if (!newvol) {
vshError(ctl, _("Failed to create vol from %s"), from);
goto cleanup;
}
+ vshPrintExtra(ctl, _("Vol %s created from input vol %s\n"),
+ virStorageVolGetName(newvol), virStorageVolGetName(inputvol));
ret = true;
cleanup:
return ret;
@@ -1147,20 +1146,19 @@ cmdVolResize(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
}
- if (virStorageVolResize(vol, capacity, flags) == 0) {
- vshPrintExtra(ctl,
- delta ? _("Size of volume '%s' successfully changed by
%s\n")
- : _("Size of volume '%s' successfully changed to
%s\n"),
- virStorageVolGetName(vol), capacityStr);
- ret = true;
- } else {
+ if (virStorageVolResize(vol, capacity, flags) < 0) {
vshError(ctl,
delta ? _("Failed to change size of volume '%s' by
%s")
: _("Failed to change size of volume '%s' to %s"),
virStorageVolGetName(vol), capacityStr);
- ret = false;
+ goto cleanup;
}
+ vshPrintExtra(ctl,
+ delta ? _("Size of volume '%s' successfully changed by
%s\n")
+ : _("Size of volume '%s' successfully changed to
%s\n"),
+ virStorageVolGetName(vol), capacityStr);
+ ret = true;
cleanup:
return ret;
}
--
2.32.0
9:42 a.m.
On a Friday in 2021, Michal Privoznik wrote:
This is all done in preparation for dropping needless labels such as:
cleanup:
return ret;
I'll post that patch as soon as these are merged.
Michal Prívozník (4):
virNetDevVPortProfileParse: Use g_autofree
examples/dommigrate: Don't set retval in usage()
examples/dommigrate: Make retval portable
virsh: Remove unnecessary else branches
examples/c/domain/dommigrate.c | 11 +++++-----
src/conf/netdev_vport_profile_conf.c | 30 +++++++++-----------------
tools/virsh-nodedev.c | 5 ++---
tools/virsh-volume.c | 32 +++++++++++++---------------
4 files changed, 32 insertions(+), 46 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:16 a.m.
On Fri, Nov 12, 2021 at 4:26 PM Michal Privoznik <mprivozn(a)redhat.com>
wrote:
This is all done in preparation for dropping needless labels such
as:
cleanup:
return ret;
I'll post that patch as soon as these are merged.
Michal Prívozník (4):
virNetDevVPortProfileParse: Use g_autofree
examples/dommigrate: Don't set retval in usage()
examples/dommigrate: Make retval portable
virsh: Remove unnecessary else branches
examples/c/domain/dommigrate.c | 11 +++++-----
src/conf/netdev_vport_profile_conf.c | 30 +++++++++-----------------
tools/virsh-nodedev.c | 5 ++---
tools/virsh-volume.c | 32 +++++++++++++---------------
4 files changed, 32 insertions(+), 46 deletions(-)
Reviewed-by: Kristína Hanicová <khanicov(a)redhat.com>
Kristína
1102
days inactive
1102
days old
6 comments
3 participants
participants (3)
-
Ján Tomko -
Kristina Hanicova -
Michal Privoznik