[libvirt PATCH 0/5] use g_auto for virCommand (Episode II.V: Goodbye, Galaxy!)
Fear not, the end is near. Ján Tomko (5): docs: use g_auto in virCommand example util: dnsmasq: refactor CapsRefresh util: iscsi: use two vars in CreateIfaceIQN util: refactor virNodeSuspendSetNodeWakeup util: use g_auto in virNodeSuspendHelper docs/internals/command.html.in | 12 +++-------- src/util/virdnsmasq.c | 37 +++++++++++++++------------------- src/util/viriscsi.c | 34 +++++++++++++++---------------- src/util/virnodesuspend.c | 16 +++------------ 4 files changed, 39 insertions(+), 60 deletions(-) -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- docs/internals/command.html.in | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in index 585ba53a93..d9f53933c6 100644 --- a/docs/internals/command.html.in +++ b/docs/internals/command.html.in @@ -564,14 +564,12 @@ int runhook(const char *drvstr, const char *id, const char *opstr, const char *subopstr, const char *extra) { - int ret; - char *path; - virCommand *cmd; + g_autofree char *path = NULL; + g_autoptr(virCommand) cmd = NULL; virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr); cmd = virCommandNew(path); - VIR_FREE(path); virCommandAddEnvPassCommon(cmd); @@ -579,11 +577,7 @@ int runhook(const char *drvstr, const char *id, virCommandSetInputBuffer(cmd, input); - ret = virCommandRun(cmd, NULL); - - virCommandFree(cmd); - - return ret; + return virCommandRun(cmd, NULL); } </pre> -- 2.31.1
Use two variables with automatic cleanup instead of reusing one. Remove the pointless cleanup label. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virdnsmasq.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 2dd9a20377..b62e353ceb 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -666,9 +666,9 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf) static int dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) { - int ret = -1; struct stat sb; - virCommand *cmd = NULL; + g_autoptr(virCommand) vercmd = NULL; + g_autoptr(virCommand) helpcmd = NULL; g_autofree char *help = NULL; g_autofree char *version = NULL; g_autofree char *complete = NULL; @@ -692,31 +692,26 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) if (!virFileIsExecutable(caps->binaryPath)) { virReportSystemError(errno, _("dnsmasq binary %s is not executable"), caps->binaryPath); - goto cleanup; + return -1; } - cmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); - virCommandSetOutputBuffer(cmd, &version); - virCommandAddEnvPassCommon(cmd); - virCommandClearCaps(cmd); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - virCommandFree(cmd); + vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); + virCommandSetOutputBuffer(vercmd, &version); + virCommandAddEnvPassCommon(vercmd); + virCommandClearCaps(vercmd); + if (virCommandRun(vercmd, NULL) < 0) + return -1; - cmd = virCommandNewArgList(caps->binaryPath, "--help", NULL); - virCommandSetOutputBuffer(cmd, &help); - virCommandAddEnvPassCommon(cmd); - virCommandClearCaps(cmd); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + helpcmd = virCommandNewArgList(caps->binaryPath, "--help", NULL); + virCommandSetOutputBuffer(helpcmd, &help); + virCommandAddEnvPassCommon(helpcmd); + virCommandClearCaps(helpcmd); + if (virCommandRun(helpcmd, NULL) < 0) + return -1; complete = g_strdup_printf("%s\n%s", version, help); - ret = dnsmasqCapsSetFromBuffer(caps, complete); - - cleanup: - virCommandFree(cmd); - return ret; + return dnsmasqCapsSetFromBuffer(caps, complete); } static dnsmasqCaps * -- 2.31.1
On 12/13/21 1:58 PM, Ján Tomko wrote:
Use two variables with automatic cleanup instead of reusing one.
Remove the pointless cleanup label.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virdnsmasq.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 2dd9a20377..b62e353ceb 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -666,9 +666,9 @@ dnsmasqCapsSetFromBuffer(dnsmasqCaps *caps, const char *buf) static int dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) { - int ret = -1; struct stat sb; - virCommand *cmd = NULL; + g_autoptr(virCommand) vercmd = NULL; + g_autoptr(virCommand) helpcmd = NULL; g_autofree char *help = NULL; g_autofree char *version = NULL; g_autofree char *complete = NULL; @@ -692,31 +692,26 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force) if (!virFileIsExecutable(caps->binaryPath)) { virReportSystemError(errno, _("dnsmasq binary %s is not executable"), caps->binaryPath); - goto cleanup; + return -1; }
- cmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); - virCommandSetOutputBuffer(cmd, &version); - virCommandAddEnvPassCommon(cmd); - virCommandClearCaps(cmd); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - virCommandFree(cmd); + vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL); + virCommandSetOutputBuffer(vercmd, &version); + virCommandAddEnvPassCommon(vercmd); + virCommandClearCaps(vercmd); + if (virCommandRun(vercmd, NULL) < 0) + return -1;
Hmmm. Every time I run across this code, I wonder if we should keep it or just remove it completely - the "newest" feature we're checking for was added to dnsmasq in version 2.67, which was released in late 2013. So all these extra executions of dnsmasq to get the version# and parse the help output are just producing the same results for everyone. On the other hand, it's possible some new feature could be added to dnsmasq in the future that we would want to check for, and that would be easier to add if the basic structure of the code was still here. I'm not sure how likely that is at this point though - dnsmasq (and libvirt's use of dnsmasq) is fairly mature at this point, so keeping the code is just creating more maintenance burden for nothing...
Do not mix automatic and manual cleanup. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/viriscsi.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index ab4363a5ab..e84d49b03c 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -208,7 +208,8 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, int exitstatus = -1; g_autofree char *iface_name = NULL; g_autofree char *temp_ifacename = NULL; - g_autoptr(virCommand) cmd = NULL; + g_autoptr(virCommand) newcmd = NULL; + g_autoptr(virCommand) updatecmd = NULL; temp_ifacename = g_strdup_printf("libvirt-iface-%08llx", (unsigned long long)virRandomBits(32)); @@ -216,35 +217,34 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'", temp_ifacename, initiatoriqn); - cmd = virCommandNewArgList(ISCSIADM, - "--mode", "iface", - "--interface", temp_ifacename, - "--op", "new", - NULL); + newcmd = virCommandNewArgList(ISCSIADM, + "--mode", "iface", + "--interface", temp_ifacename, + "--op", "new", + NULL); /* Note that we ignore the exitstatus. Older versions of iscsiadm * tools returned an exit status of > 0, even if they succeeded. * We will just rely on whether the interface got created * properly. */ - if (virCommandRun(cmd, &exitstatus) < 0) { + if (virCommandRun(newcmd, &exitstatus) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to run command '%s' to create new iscsi interface"), ISCSIADM); return -1; } - virCommandFree(cmd); - cmd = virCommandNewArgList(ISCSIADM, - "--mode", "iface", - "--interface", temp_ifacename, - "--op", "update", - "--name", "iface.initiatorname", - "--value", - initiatoriqn, - NULL); + updatecmd = virCommandNewArgList(ISCSIADM, + "--mode", "iface", + "--interface", temp_ifacename, + "--op", "update", + "--name", "iface.initiatorname", + "--value", + initiatoriqn, + NULL); /* Note that we ignore the exitstatus. Older versions of iscsiadm tools * returned an exit status of > 0, even if they succeeded. We will just * rely on whether iface file got updated properly. */ - if (virCommandRun(cmd, &exitstatus) < 0) { + if (virCommandRun(updatecmd, &exitstatus) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), ISCSIADM, initiatoriqn); -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virnodesuspend.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index 451d798363..c92232dfe9 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -73,8 +73,7 @@ static void virNodeSuspendUnlock(void) */ static int virNodeSuspendSetNodeWakeup(unsigned long long alarmTime) { - virCommand *setAlarmCmd; - int ret = -1; + g_autoptr(virCommand) setAlarmCmd = NULL; if (alarmTime < MIN_TIME_REQ_FOR_SUSPEND) { virReportError(VIR_ERR_INVALID_ARG, @@ -86,14 +85,7 @@ static int virNodeSuspendSetNodeWakeup(unsigned long long alarmTime) setAlarmCmd = virCommandNewArgList("rtcwake", "-m", "no", "-s", NULL); virCommandAddArgFormat(setAlarmCmd, "%lld", alarmTime); - if (virCommandRun(setAlarmCmd, NULL) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virCommandFree(setAlarmCmd); - return ret; + return virCommandRun(setAlarmCmd, NULL); } /** -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virnodesuspend.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c index c92232dfe9..e1167605ca 100644 --- a/src/util/virnodesuspend.c +++ b/src/util/virnodesuspend.c @@ -102,7 +102,7 @@ static int virNodeSuspendSetNodeWakeup(unsigned long long alarmTime) */ static void virNodeSuspendHelper(void *cmdString) { - virCommand *suspendCmd = virCommandNew((const char *)cmdString); + g_autoptr(virCommand) suspendCmd = virCommandNew((const char *)cmdString); /* * Delay for sometime so that the function virNodeSuspend() @@ -112,8 +112,6 @@ static void virNodeSuspendHelper(void *cmdString) if (virCommandRun(suspendCmd, NULL) < 0) VIR_WARN("Failed to suspend the host"); - virCommandFree(suspendCmd); - /* * Now that we have resumed from suspend or the suspend failed, * reset 'aboutToSuspend' flag. -- 2.31.1
On 12/13/21 19:58, Ján Tomko wrote:
Fear not, the end is near.
Ján Tomko (5): docs: use g_auto in virCommand example util: dnsmasq: refactor CapsRefresh util: iscsi: use two vars in CreateIfaceIQN util: refactor virNodeSuspendSetNodeWakeup util: use g_auto in virNodeSuspendHelper
docs/internals/command.html.in | 12 +++-------- src/util/virdnsmasq.c | 37 +++++++++++++++------------------- src/util/viriscsi.c | 34 +++++++++++++++---------------- src/util/virnodesuspend.c | 16 +++------------ 4 files changed, 39 insertions(+), 60 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Ján Tomko -
Laine Stump -
Michal Prívozník