Roger Pau Monne
2013-Nov-21 16:17 UTC
[PATCH 0/2] libxl: fixes for coverity reported issues
Fixes for two bugs coverity found after the driver domain series went in.
Coverity-ID: 1130521
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4292c78..ed32931 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3596,14 +3596,14 @@ static void device_complete(libxl__egc *egc,
libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
- if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE)
- free(aodev->dev);
-
LOG(DEBUG, "device %s %s %s",
libxl__device_backend_path(gc, aodev->dev),
libxl__device_action_to_string(aodev->action),
aodev->rc ? "failed" : "succeed");
+ if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE)
+ free(aodev->dev);
+
libxl__nested_ao_free(aodev->ao);
}
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Roger Pau Monne
2013-Nov-21 16:18 UTC
[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error
Coverity-ID: 1130517 and 1130518
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dm.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 292e351..469b82e 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1393,7 +1393,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
dmss->spawn.detached_cb = device_model_detached;
rc = libxl__spawn_spawn(egc, &dmss->spawn);
if (rc < 0)
- goto error;
+ goto error_close;
if (!rc) { /* inner child */
setsid();
libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
@@ -1401,6 +1401,9 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
return;
+error_close:
+ close(null);
+ close(logfile_w);
error:
assert(rc);
dmss->callback(egc, dmss, rc);
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Andrew Cooper
2013-Nov-21 17:19 UTC
Re: [PATCH 0/2] libxl: fixes for coverity reported issues
On 21/11/13 16:17, Roger Pau Monne wrote:> Fixes for two bugs coverity found after the driver domain series went > in.Both Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> as I did the initial coverity triage. Roger: Are you going to do the daemonise issues or shall I? ~Andrew> _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Roger Pau Monne writes ("[PATCH 1/2] libxl: fix use of aodev->dev after
free"):> Coverity-ID: 1130521
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
Thanks,
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
And added to my backport list.
Ian.
Ian Jackson
2013-Nov-21 18:42 UTC
Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error
Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has
to close opened files on error"):> Coverity-ID: 1130517 and 1130518
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
I''m don''t think that''s the right fix. I think the
fds are leaked in
the success case too. How about this ?
From c2b3a5a9225398e6c2381d34ff2d6e9aa07d74fa Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu, 21 Nov 2013 18:37:16 +0000
Subject: [PATCH] libxl: libxl__spawn_qdisk_backend closes fds
This function needs to close both null and logfile_w on both error and
normal exits. (The child gets its own copy during the fork, and the
parent doesn''t need them any more.)
Use the standard initialise-to-unallocated, always-free style. As a
result the label "error" becomes "out", and only makes the
callback if
rc is nonzero.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Coverity-ID: 1130517 and 1130518
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dm.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 292e351..7e37385 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1343,7 +1343,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
flexarray_t *dm_args;
char **args;
const char *dm;
- int logfile_w, null, rc;
+ int logfile_w = -1, null = -1, rc;
uint32_t domid = dmss->guest_domid;
/* Always use qemu-xen as device model */
@@ -1393,17 +1393,22 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
dmss->spawn.detached_cb = device_model_detached;
rc = libxl__spawn_spawn(egc, &dmss->spawn);
if (rc < 0)
- goto error;
+ goto out;
if (!rc) { /* inner child */
setsid();
libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
}
- return;
+ rc = 0;
-error:
- assert(rc);
- dmss->callback(egc, dmss, rc);
+ out:
+ if (logfile_w >= 0) close(logfile_w);
+ if (null >= 0) close(null);
+
+ /* rc is nonzero iff we had an error; if we had no error then
+ * spawn succeeded and we will continue in a further callback */
+ if (rc)
+ dmss->callback(egc, dmss, rc);
return;
}
--
1.7.10.4
Ian Jackson
2013-Nov-21 18:50 UTC
Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error
Ian Jackson writes ("Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has
to close opened files on error"):> Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend
has to close opened files on error"):
> > Coverity-ID: 1130517 and 1130518
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> I''m don''t think that''s the right fix. I think
the fds are leaked in
> the success case too. How about this ?
Maybe you''d prefer a version which at least compiles...
Ian.
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu, 21 Nov 2013 18:37:16 +0000
Subject: [PATCH v2] libxl: libxl__spawn_qdisk_backend closes fds
This function needs to close both null and logfile_w on both error and
normal exits. (The child gets its own copy during the fork, and the
parent doesn''t need them any more.)
Use the standard initialise-to-unallocated, always-free style. As a
result the label "error" becomes "out", and only makes the
callback if
rc is nonzero.
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Coverity-ID: 1130517 and 1130518
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl_dm.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 292e351..548378d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1343,7 +1343,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
flexarray_t *dm_args;
char **args;
const char *dm;
- int logfile_w, null, rc;
+ int logfile_w = -1, null = -1, rc;
uint32_t domid = dmss->guest_domid;
/* Always use qemu-xen as device model */
@@ -1366,7 +1366,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u",
domid));
if (logfile_w < 0) {
rc = logfile_w;
- goto error;
+ goto out;
}
null = open("/dev/null", O_RDONLY);
@@ -1393,17 +1393,22 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
dmss->spawn.detached_cb = device_model_detached;
rc = libxl__spawn_spawn(egc, &dmss->spawn);
if (rc < 0)
- goto error;
+ goto out;
if (!rc) { /* inner child */
setsid();
libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
}
- return;
+ rc = 0;
-error:
- assert(rc);
- dmss->callback(egc, dmss, rc);
+ out:
+ if (logfile_w >= 0) close(logfile_w);
+ if (null >= 0) close(null);
+
+ /* rc is nonzero iff we had an error; if we had no error then
+ * spawn succeeded and we will continue in a further callback */
+ if (rc)
+ dmss->callback(egc, dmss, rc);
return;
}
--
1.7.10.4
Andrew Cooper
2013-Nov-21 19:04 UTC
Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error
On 21/11/13 18:50, Ian Jackson wrote:> Ian Jackson writes ("Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): >> Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): >>> Coverity-ID: 1130517 and 1130518 >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> I''m don''t think that''s the right fix. I think the fds are leaked in >> the success case too. How about this ? > Maybe you''d prefer a version which at least compiles... > > Ian. > > From: Ian Jackson <ian.jackson@eu.citrix.com> > Date: Thu, 21 Nov 2013 18:37:16 +0000 > Subject: [PATCH v2] libxl: libxl__spawn_qdisk_backend closes fds > > This function needs to close both null and logfile_w on both error and > normal exits. (The child gets its own copy during the fork, and the > parent doesn''t need them any more.) > > Use the standard initialise-to-unallocated, always-free style. As a > result the label "error" becomes "out", and only makes the callback if > rc is nonzero. > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > Coverity-ID: 1130517 and 1130518 > Cc: Roger Pau Monne <roger.pau@citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com>Right - this clarifies my question about cleanup on the success case.> --- > tools/libxl/libxl_dm.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 292e351..548378d 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -1343,7 +1343,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > flexarray_t *dm_args; > char **args; > const char *dm; > - int logfile_w, null, rc; > + int logfile_w = -1, null = -1, rc;The rc logic is a little awkward. Would it be better to initialise to -1 here...> uint32_t domid = dmss->guest_domid; > > /* Always use qemu-xen as device model */ > @@ -1366,7 +1366,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u", domid)); > if (logfile_w < 0) { > rc = logfile_w;... and avoid this somewhat odd assignment? ~Andrew> - goto error; > + goto out; > } > null = open("/dev/null", O_RDONLY); > > @@ -1393,17 +1393,22 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > dmss->spawn.detached_cb = device_model_detached; > rc = libxl__spawn_spawn(egc, &dmss->spawn); > if (rc < 0) > - goto error; > + goto out; > if (!rc) { /* inner child */ > setsid(); > libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL); > } > > - return; > + rc = 0; > > -error: > - assert(rc); > - dmss->callback(egc, dmss, rc); > + out: > + if (logfile_w >= 0) close(logfile_w); > + if (null >= 0) close(null); > + > + /* rc is nonzero iff we had an error; if we had no error then > + * spawn succeeded and we will continue in a further callback */ > + if (rc) > + dmss->callback(egc, dmss, rc); > return; > } >
Andrew Cooper
2013-Nov-21 19:07 UTC
Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error
On 21/11/13 19:04, Andrew Cooper wrote:> On 21/11/13 18:50, Ian Jackson wrote: >> Ian Jackson writes ("Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): >>> Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): >>>> Coverity-ID: 1130517 and 1130518 >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> I''m don''t think that''s the right fix. I think the fds are leaked in >>> the success case too. How about this ? >> Maybe you''d prefer a version which at least compiles... >> >> Ian. >> >> From: Ian Jackson <ian.jackson@eu.citrix.com> >> Date: Thu, 21 Nov 2013 18:37:16 +0000 >> Subject: [PATCH v2] libxl: libxl__spawn_qdisk_backend closes fds >> >> This function needs to close both null and logfile_w on both error and >> normal exits. (The child gets its own copy during the fork, and the >> parent doesn''t need them any more.) >> >> Use the standard initialise-to-unallocated, always-free style. As a >> result the label "error" becomes "out", and only makes the callback if >> rc is nonzero. >> >> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> >> Coverity-ID: 1130517 and 1130518 >> Cc: Roger Pau Monne <roger.pau@citrix.com> >> Cc: Ian Campbell <ian.campbell@citrix.com> > Right - this clarifies my question about cleanup on the success case. > >> --- >> tools/libxl/libxl_dm.c | 19 ++++++++++++------- >> 1 file changed, 12 insertions(+), 7 deletions(-) >> >> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c >> index 292e351..548378d 100644 >> --- a/tools/libxl/libxl_dm.c >> +++ b/tools/libxl/libxl_dm.c >> @@ -1343,7 +1343,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) >> flexarray_t *dm_args; >> char **args; >> const char *dm; >> - int logfile_w, null, rc; >> + int logfile_w = -1, null = -1, rc; > The rc logic is a little awkward. Would it be better to initialise to > -1 here... > >> uint32_t domid = dmss->guest_domid; >> >> /* Always use qemu-xen as device model */ >> @@ -1366,7 +1366,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) >> logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u", domid)); >> if (logfile_w < 0) { >> rc = logfile_w; > ... and avoid this somewhat odd assignment? > > ~Andrew > >> - goto error; >> + goto out; >> } >> null = open("/dev/null", O_RDONLY);And thinking about it, this open() should also have some error checking. ~Andrew>> >> @@ -1393,17 +1393,22 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) >> dmss->spawn.detached_cb = device_model_detached; >> rc = libxl__spawn_spawn(egc, &dmss->spawn); >> if (rc < 0) >> - goto error; >> + goto out; >> if (!rc) { /* inner child */ >> setsid(); >> libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL); >> } >> >> - return; >> + rc = 0; >> >> -error: >> - assert(rc); >> - dmss->callback(egc, dmss, rc); >> + out: >> + if (logfile_w >= 0) close(logfile_w); >> + if (null >= 0) close(null); >> + >> + /* rc is nonzero iff we had an error; if we had no error then >> + * spawn succeeded and we will continue in a further callback */ >> + if (rc) >> + dmss->callback(egc, dmss, rc); >> return; >> } >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Roger Pau Monné
2013-Nov-22 11:45 UTC
Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error
On 21/11/13 19:50, Ian Jackson wrote:> Ian Jackson writes ("Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): >> Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): >>> Coverity-ID: 1130517 and 1130518 >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> >> I''m don''t think that''s the right fix. I think the fds are leaked in >> the success case too. How about this ? > > Maybe you''d prefer a version which at least compiles... > > Ian. > > From: Ian Jackson <ian.jackson@eu.citrix.com> > Date: Thu, 21 Nov 2013 18:37:16 +0000 > Subject: [PATCH v2] libxl: libxl__spawn_qdisk_backend closes fds > > This function needs to close both null and logfile_w on both error and > normal exits. (The child gets its own copy during the fork, and the > parent doesn''t need them any more.) > > Use the standard initialise-to-unallocated, always-free style. As a > result the label "error" becomes "out", and only makes the callback if > rc is nonzero. > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > Coverity-ID: 1130517 and 1130518 > Cc: Roger Pau Monne <roger.pau@citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > --- > tools/libxl/libxl_dm.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 292e351..548378d 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -1343,7 +1343,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > flexarray_t *dm_args; > char **args; > const char *dm; > - int logfile_w, null, rc; > + int logfile_w = -1, null = -1, rc; > uint32_t domid = dmss->guest_domid; > > /* Always use qemu-xen as device model */ > @@ -1366,7 +1366,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u", domid)); > if (logfile_w < 0) { > rc = logfile_w; > - goto error; > + goto out; > } > null = open("/dev/null", O_RDONLY); > > @@ -1393,17 +1393,22 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > dmss->spawn.detached_cb = device_model_detached; > rc = libxl__spawn_spawn(egc, &dmss->spawn); > if (rc < 0) > - goto error; > + goto out; > if (!rc) { /* inner child */ > setsid(); > libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL); > } > > - return; > + rc = 0; > > -error: > - assert(rc); > - dmss->callback(egc, dmss, rc); > + out: > + if (logfile_w >= 0) close(logfile_w); > + if (null >= 0) close(null); > + > + /* rc is nonzero iff we had an error; if we had no error then > + * spawn succeeded and we will continue in a further callback */ > + if (rc) > + dmss->callback(egc, dmss, rc);I didn''t catch that when modifying the original patch to be more similar to libxl__spawn_local_dm, but IMHO you could use device_model_spawn_outcome here which will print a nice error message about the device model is unable to start. Apart from this (that can be done in a separate patch): Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Ian Campbell
2013-Nov-29 09:58 UTC
Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error
On Thu, 2013-11-21 at 19:04 +0000, Andrew Cooper wrote:> On 21/11/13 18:50, Ian Jackson wrote: > > Ian Jackson writes ("Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): > >> Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): > >>> Coverity-ID: 1130517 and 1130518 > >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> I'm don't think that's the right fix. I think the fds are leaked in > >> the success case too. How about this ? > > Maybe you'd prefer a version which at least compiles... > > > > Ian. > > > > From: Ian Jackson <ian.jackson@eu.citrix.com> > > Date: Thu, 21 Nov 2013 18:37:16 +0000 > > Subject: [PATCH v2] libxl: libxl__spawn_qdisk_backend closes fds > > > > This function needs to close both null and logfile_w on both error and > > normal exits. (The child gets its own copy during the fork, and the > > parent doesn't need them any more.) > > > > Use the standard initialise-to-unallocated, always-free style. As a > > result the label "error" becomes "out", and only makes the callback if > > rc is nonzero. > > > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > > Coverity-ID: 1130517 and 1130518 > > Cc: Roger Pau Monne <roger.pau@citrix.com> > > Cc: Ian Campbell <ian.campbell@citrix.com> > > Right - this clarifies my question about cleanup on the success case. > > > --- > > tools/libxl/libxl_dm.c | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > index 292e351..548378d 100644 > > --- a/tools/libxl/libxl_dm.c > > +++ b/tools/libxl/libxl_dm.c > > @@ -1343,7 +1343,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > > flexarray_t *dm_args; > > char **args; > > const char *dm; > > - int logfile_w, null, rc; > > + int logfile_w = -1, null = -1, rc; > > The rc logic is a little awkward. Would it be better to initialise to > -1 here... > > > uint32_t domid = dmss->guest_domid; > > > > /* Always use qemu-xen as device model */ > > @@ -1366,7 +1366,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > > logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u", domid)); > > if (logfile_w < 0) { > > rc = logfile_w; > > ... and avoid this somewhat odd assignment?rc is traditionally a libxl ERROR_FOO not -1. libxl__create_qemu_logfile returns one of those or a valid fd >= 0.> > ~Andrew > > > - goto error; > > + goto out; > > } > > null = open("/dev/null", O_RDONLY); > > > > @@ -1393,17 +1393,22 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > > dmss->spawn.detached_cb = device_model_detached; > > rc = libxl__spawn_spawn(egc, &dmss->spawn); > > if (rc < 0) > > - goto error; > > + goto out; > > if (!rc) { /* inner child */ > > setsid(); > > libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL); > > } > > > > - return; > > + rc = 0; > > > > -error: > > - assert(rc); > > - dmss->callback(egc, dmss, rc); > > + out: > > + if (logfile_w >= 0) close(logfile_w); > > + if (null >= 0) close(null); > > + > > + /* rc is nonzero iff we had an error; if we had no error then > > + * spawn succeeded and we will continue in a further callback */ > > + if (rc) > > + dmss->callback(egc, dmss, rc); > > return; > > } > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2013-Nov-29 10:04 UTC
Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error
On Fri, 2013-11-22 at 12:45 +0100, Roger Pau Monné wrote:> On 21/11/13 19:50, Ian Jackson wrote: > > Ian Jackson writes ("Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): > >> Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error"): > >>> Coverity-ID: 1130517 and 1130518 > >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> > >> I'm don't think that's the right fix. I think the fds are leaked in > >> the success case too. How about this ? > > > > Maybe you'd prefer a version which at least compiles... > > > > Ian. > > > > From: Ian Jackson <ian.jackson@eu.citrix.com> > > Date: Thu, 21 Nov 2013 18:37:16 +0000 > > Subject: [PATCH v2] libxl: libxl__spawn_qdisk_backend closes fds > > > > This function needs to close both null and logfile_w on both error and > > normal exits. (The child gets its own copy during the fork, and the > > parent doesn't need them any more.) > > > > Use the standard initialise-to-unallocated, always-free style. As a > > result the label "error" becomes "out", and only makes the callback if > > rc is nonzero. > > > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > > Coverity-ID: 1130517 and 1130518 > > Cc: Roger Pau Monne <roger.pau@citrix.com> > > Cc: Ian Campbell <ian.campbell@citrix.com> > > --- > > tools/libxl/libxl_dm.c | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > > index 292e351..548378d 100644 > > --- a/tools/libxl/libxl_dm.c > > +++ b/tools/libxl/libxl_dm.c > > @@ -1343,7 +1343,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > > flexarray_t *dm_args; > > char **args; > > const char *dm; > > - int logfile_w, null, rc; > > + int logfile_w = -1, null = -1, rc; > > uint32_t domid = dmss->guest_domid; > > > > /* Always use qemu-xen as device model */ > > @@ -1366,7 +1366,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > > logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u", domid)); > > if (logfile_w < 0) { > > rc = logfile_w; > > - goto error; > > + goto out; > > } > > null = open("/dev/null", O_RDONLY); > > > > @@ -1393,17 +1393,22 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) > > dmss->spawn.detached_cb = device_model_detached; > > rc = libxl__spawn_spawn(egc, &dmss->spawn); > > if (rc < 0) > > - goto error; > > + goto out; > > if (!rc) { /* inner child */ > > setsid(); > > libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL); > > } > > > > - return; > > + rc = 0; > > > > -error: > > - assert(rc); > > - dmss->callback(egc, dmss, rc); > > + out: > > + if (logfile_w >= 0) close(logfile_w); > > + if (null >= 0) close(null); > > + > > + /* rc is nonzero iff we had an error; if we had no error then > > + * spawn succeeded and we will continue in a further callback */ > > + if (rc) > > + dmss->callback(egc, dmss, rc); > > I didn't catch that when modifying the original patch to be more similar > to libxl__spawn_local_dm, but IMHO you could use > device_model_spawn_outcome here which will print a nice error message > about the device model is unable to start. Apart from this (that can be > done in a separate patch): > > Acked-by: Roger Pau Monné <roger.pau@citrix.com>The version of the patch at http://bugs.xenproject.org/xen/mid/% 3C21134.22006.356685.408733@mariner.uk.xensource.com%3E/raw (which should be reasonably unmangled) has gotten quoted printable'd somewhere. Would also be good to add the error handling on opening of null as well. Currently on failure there libxl__exec will translate the -1 into STDxxx_FILENO. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel