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
Explicit calls to VIR_FREE() can be dropped then. Signed-off-by: Michal Privoznik <mprivozn@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
The usage() function should just print expected arguments. Make the function return void then. Signed-off-by: Michal Privoznik <mprivozn@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
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@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
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@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
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@redhat.com> Jano
On Fri, Nov 12, 2021 at 4:26 PM Michal Privoznik <mprivozn@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@redhat.com> Kristína
participants (3)
-
Ján Tomko -
Kristina Hanicova -
Michal Privoznik