6:34 a.m.
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
6:34 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Refactor qemuParseCommandLine error path for vnc parse
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(a)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
9:35 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Refactor qemuParseCommandLine error path for vnc parse
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(a)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
9:43 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Refactor qemuParseCommandLine error path for vnc parse
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(a)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
6:34 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check return status for virDomainGraphicsListenSetAddress
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(a)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
9:43 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Check return status for virDomainGraphicsListenSetAddress
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(a)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
3242
days inactive
3242
days old
5 comments
3 participants
participants (3)
-
Cole Robinson -
John Ferlan -
Ján Tomko