[libvirt] [PATCH 0/2] Check return status for virDomainGraphicsListenSetAddress
The second patch is a result of a Coverity found issue - the first one is collateral damage found because my first pass just checked the status and went to error resulting in a resource leak.... Rather than add two more lines for calling the graphic def free function, it was just easier to refactor the code. John Ferlan (2): qemu: Refactor qemuParseCommandLine error path for vnc parse qemu: Check return status for virDomainGraphicsListenSetAddress src/qemu/qemu_command.c | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-) -- 2.5.0
Rather than have vnc be a variable within the if, promote it to the top, then adjust the code to use the error label to call virDomainGraphicsDefFree Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d7f19f3..7b5a36f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -12999,6 +12999,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; virDomainDiskDefPtr disk = NULL; + virDomainGraphicsDefPtr vnc = NULL; const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS"); bool have_sdl = false; @@ -13100,7 +13101,6 @@ qemuParseCommandLine(virCapsPtr qemuCaps, arg++; if (STREQ(arg, "-vnc")) { - virDomainGraphicsDefPtr vnc; char *tmp; WANT_VALUE(); if (VIR_ALLOC(vnc) < 0) @@ -13109,10 +13109,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps, if (STRPREFIX(val, "unix:")) { /* -vnc unix:/some/big/path */ - if (VIR_STRDUP(vnc->data.vnc.socket, val + 5) < 0) { - virDomainGraphicsDefFree(vnc); + if (VIR_STRDUP(vnc->data.vnc.socket, val + 5) < 0) goto error; - } } else { /* * -vnc 127.0.0.1:4 @@ -13126,7 +13124,6 @@ qemuParseCommandLine(virCapsPtr qemuCaps, sep = "]:"; tmp = strstr(val, sep); if (!tmp) { - virDomainGraphicsDefFree(vnc); virReportError(VIR_ERR_INTERNAL_ERROR, _("missing VNC port number in '%s'"), val); goto error; @@ -13134,7 +13131,6 @@ qemuParseCommandLine(virCapsPtr qemuCaps, port = tmp + strlen(sep); if (virStrToLong_i(port, &opts, 10, &vnc->data.vnc.port) < 0) { - virDomainGraphicsDefFree(vnc); virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse VNC port '%s'"), port); goto error; @@ -13145,18 +13141,14 @@ qemuParseCommandLine(virCapsPtr qemuCaps, else virDomainGraphicsListenSetAddress(vnc, 0, val, tmp-val, true); - if (!virDomainGraphicsListenGetAddress(vnc, 0)) { - virDomainGraphicsDefFree(vnc); + if (!virDomainGraphicsListenGetAddress(vnc, 0)) goto error; - } if (*opts == ',') { char *orig_opts; - if (VIR_STRDUP(orig_opts, opts + 1) < 0) { - virDomainGraphicsDefFree(vnc); + if (VIR_STRDUP(orig_opts, opts + 1) < 0) goto error; - } opts = orig_opts; while (opts && *opts) { @@ -13177,7 +13169,6 @@ qemuParseCommandLine(virCapsPtr qemuCaps, _("cannot parse VNC " "WebSocket port '%s'"), websocket); - virDomainGraphicsDefFree(vnc); VIR_FREE(orig_opts); goto error; } @@ -13198,7 +13189,6 @@ qemuParseCommandLine(virCapsPtr qemuCaps, virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown vnc display sharing policy '%s'"), sharePolicy); - virDomainGraphicsDefFree(vnc); VIR_FREE(orig_opts); goto error; } else { @@ -13207,7 +13197,6 @@ qemuParseCommandLine(virCapsPtr qemuCaps, } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing vnc sharing policy")); - virDomainGraphicsDefFree(vnc); VIR_FREE(orig_opts); goto error; } @@ -13221,10 +13210,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps, vnc->data.vnc.autoport = false; } - if (VIR_APPEND_ELEMENT(def->graphics, def->ngraphics, vnc) < 0) { - virDomainGraphicsDefFree(vnc); + if (VIR_APPEND_ELEMENT(def->graphics, def->ngraphics, vnc) < 0) goto error; - } } else if (STREQ(arg, "-sdl")) { have_sdl = true; } else if (STREQ(arg, "-m")) { @@ -13991,6 +13978,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, return def; error: + virDomainGraphicsDefFree(vnc); virDomainDiskDefFree(disk); qemuDomainCmdlineDefFree(cmd); virDomainDefFree(def); -- 2.5.0
On Tue, Feb 09, 2016 at 07:34:01AM -0500, John Ferlan wrote:
Rather than have vnc be a variable within the if, promote it to the top, then adjust the code to use the error label to call virDomainGraphicsDefFree
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d7f19f3..7b5a36f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -12999,6 +12999,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; virDomainDiskDefPtr disk = NULL; + virDomainGraphicsDefPtr vnc = NULL; const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS"); bool have_sdl = false;
[...]
@@ -13991,6 +13978,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, return def;
error: + virDomainGraphicsDefFree(vnc); virDomainDiskDefFree(disk); qemuDomainCmdlineDefFree(cmd); virDomainDefFree(def);
The Free call is almost a thousand lines after the declaration. This is a great opportunity to split out the VNC parsing into a separate function, especially since it seems that its only input is a const char* pointing to the command line option and the only output is one virDomainGraphicsDef structure. Jan
On 02/09/2016 10:35 AM, Ján Tomko wrote:
On Tue, Feb 09, 2016 at 07:34:01AM -0500, John Ferlan wrote:
Rather than have vnc be a variable within the if, promote it to the top, then adjust the code to use the error label to call virDomainGraphicsDefFree
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d7f19f3..7b5a36f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -12999,6 +12999,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, int nvirtiodisk = 0; qemuDomainCmdlineDefPtr cmd = NULL; virDomainDiskDefPtr disk = NULL; + virDomainGraphicsDefPtr vnc = NULL; const char *ceph_args = qemuFindEnv(progenv, "CEPH_ARGS"); bool have_sdl = false;
[...]
@@ -13991,6 +13978,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, return def;
error: + virDomainGraphicsDefFree(vnc); virDomainDiskDefFree(disk); qemuDomainCmdlineDefFree(cmd); virDomainDefFree(def);
The Free call is almost a thousand lines after the declaration. This is a great opportunity to split out the VNC parsing into a separate function, especially since it seems that its only input is a const char* pointing to the command line option and the only output is one virDomainGraphicsDef structure.
Doubly nice: move all this code to its own file like qemu_command_parse.c . AFAICT it has very little overlap with the rest of the (quite large) qemu_command.c - Cole
Recent refactors in the vbox code to check the return status for the function tipped Coverity's scales of justice for any functions that do not check status - such as this one. Add a check of the status, adjust the long line too. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7b5a36f..a0ff5d4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -13135,12 +13135,17 @@ qemuParseCommandLine(virCapsPtr qemuCaps, _("cannot parse VNC port '%s'"), port); goto error; } - if (val[0] == '[') - virDomainGraphicsListenSetAddress(vnc, 0, - val+1, tmp-(val+1), true); - else - virDomainGraphicsListenSetAddress(vnc, 0, - val, tmp-val, true); + if (val[0] == '[') { + if (virDomainGraphicsListenSetAddress(vnc, 0, + val+1, tmp-(val+1), + true) < 0) + goto error; + } else { + if (virDomainGraphicsListenSetAddress(vnc, 0, + val, tmp-val, + true) < 0) + goto error; + } if (!virDomainGraphicsListenGetAddress(vnc, 0)) goto error; -- 2.5.0
On Tue, Feb 09, 2016 at 07:34:02AM -0500, John Ferlan wrote:
Recent refactors in the vbox code to check the return status for the function tipped Coverity's scales of justice for any functions that do not check status - such as this one.
Add a check of the status, adjust the long line too.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7b5a36f..a0ff5d4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -13135,12 +13135,17 @@ qemuParseCommandLine(virCapsPtr qemuCaps, _("cannot parse VNC port '%s'"), port); goto error; } - if (val[0] == '[') - virDomainGraphicsListenSetAddress(vnc, 0, - val+1, tmp-(val+1), true); - else - virDomainGraphicsListenSetAddress(vnc, 0, - val, tmp-val, true); + if (val[0] == '[') { + if (virDomainGraphicsListenSetAddress(vnc, 0, + val+1, tmp-(val+1), + true) < 0) + goto error; + } else { + if (virDomainGraphicsListenSetAddress(vnc, 0, + val, tmp-val, + true) < 0) + goto error; + }
if (!virDomainGraphicsListenGetAddress(vnc, 0)) goto error;
This call looks redundant after we start checking the return value. Looking at commit ef79fb5b5 [1] which introduced this, the code was: if (val[0] == '[') vnc->data.vnc.listenAddr = strndup(val+1, tmp-(val+1)); else vnc->data.vnc.listenAddr = strndup(val, tmp-val); if (!vnc->data.vnc.listenAddr) { VIR_FREE(vnc); goto no_memory; } so it's just a leftover from strndup -> ListenSetAddress conversion. Jan [1] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=ef79fb5b5
participants (3)
-
Cole Robinson -
John Ferlan -
Ján Tomko