12:58 p.m.
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
12:58 p.m.
New subject: [libvirt PATCH 1/5] docs: use g_auto in virCommand example
Signed-off-by: Ján Tomko <jtomko(a)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
12:58 p.m.
New subject: [libvirt PATCH 2/5] util: dnsmasq: refactor CapsRefresh
Use two variables with automatic cleanup instead of reusing one.
Remove the pointless cleanup label.
Signed-off-by: Ján Tomko <jtomko(a)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
8:56 a.m.
New subject: [libvirt PATCH 2/5] util: dnsmasq: refactor CapsRefresh
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(a)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...
12:58 p.m.
New subject: [libvirt PATCH 3/5] util: iscsi: use two vars in CreateIfaceIQN
Do not mix automatic and manual cleanup.
Signed-off-by: Ján Tomko <jtomko(a)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
12:58 p.m.
New subject: [libvirt PATCH 4/5] util: refactor virNodeSuspendSetNodeWakeup
Signed-off-by: Ján Tomko <jtomko(a)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
12:58 p.m.
New subject: [libvirt PATCH 5/5] util: use g_auto in virNodeSuspendHelper
Signed-off-by: Ján Tomko <jtomko(a)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
5:49 a.m.
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(a)redhat.com>
Michal
1073
days inactive
1074
days old
7 comments
3 participants
participants (3)
-
Ján Tomko -
Laine Stump -
Michal Prívozník