[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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@xxxxxxxxxx> >> >> 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@xxxxxxxxxxxxx> > 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@xxxxxxxxxxxxx> > Coverity-ID: 1130517 and 1130518 > Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx> > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |