9:28 a.m.
When returning early due to errors, cmd will be leaked. Use an autoptr
to handle these early returns without leaking memory.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index b0cc63ed42..e565cc29ec 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -724,7 +724,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
char **errbuf)
{
g_autofree char *parent_addr = NULL;
- virCommand *cmd = NULL;
+ g_autoptr(virCommand) cmd = NULL;
const char *subcommand = virMdevctlCommandTypeToString(cmd_type);
g_autofree char *inbuf = NULL;
@@ -787,7 +787,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
virCommandSetErrorBuffer(cmd, errbuf);
- return cmd;
+ return g_steal_pointer(&cmd);
}
--
2.26.3
9:28 a.m.
New subject: [libvirt PATCH 2/2] nodedev: handle failure to generate mdevctl cmd
Coverity complained that the 'default' case of the switch in
nodeDeviceGetMdevctlCommand() was falling through without initializing
'cmd'. Return NULL in this case even though it should never happen.
Also, make sure calling functions handle the NULL return consistently.
Some callers already handled it, but some didn't.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index e565cc29ec..c1fe9db149 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -743,7 +743,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
case MDEVCTL_CMD_LAST:
default:
/* SHOULD NEVER HAPPEN */
- break;
+ return NULL;
}
switch (cmd_type) {
@@ -931,6 +931,9 @@ virMdevctlStop(virNodeDeviceDef *def, char **errmsg)
cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg);
+ if (!cmd)
+ return -1;
+
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
@@ -946,6 +949,9 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg)
cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, errmsg);
+ if (!cmd)
+ return -1;
+
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
@@ -961,6 +967,9 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg)
cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, errmsg);
+ if (!cmd)
+ return -1;
+
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
--
2.26.3
2:43 a.m.
New subject: [libvirt PATCH 2/2] nodedev: handle failure to generate mdevctl cmd
On 4/20/21 4:28 PM, Jonathon Jongsma wrote:
Coverity complained that the 'default' case of the switch in
nodeDeviceGetMdevctlCommand() was falling through without initializing
'cmd'. Return NULL in this case even though it should never happen.
Also, make sure calling functions handle the NULL return consistently.
Some callers already handled it, but some didn't.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index e565cc29ec..c1fe9db149 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -743,7 +743,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
case MDEVCTL_CMD_LAST:
default:
/* SHOULD NEVER HAPPEN */
- break;
+ return NULL;
}
switch (cmd_type) {
ACK to this hunk.
@@ -931,6 +931,9 @@ virMdevctlStop(virNodeDeviceDef *def, char
**errmsg)
cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg);
+ if (!cmd)
+ return -1;
+
But this one and the rest is redundant IMO. The whole design of
virCommand wraps around caller not checking NULL. For instance the
following is perfectly valid:
virCommandPtr cmd = virCommandNew("someBinary");
virCommandAddArg(cmd, "arg1");
virCommandPassFD(cmd, 1);
if (virCommandRun(cmd, NULL) < 0) {
/* error */
}
if (virCommandRun(cmd, &status) < 0 || status != 0)
Therefore, if @cmd was NULL then this virCommandRun() returns -1 with
appropriate error message set. I think instead of introducing these new
checks the old ones should be removed.
Michal
2:44 a.m.
On 4/20/21 4:28 PM, Jonathon Jongsma wrote:
When returning early due to errors, cmd will be leaked. Use an
autoptr
to handle these early returns without leaking memory.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
1310
days inactive
1311
days old
3 comments
2 participants
participants (2)
-
Jonathon Jongsma -
Michal Privoznik