From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Subject: Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error Date: Fri, 22 Nov 2013 12:45:17 +0100 Message-ID: <528F43CD.3090407@citrix.com> References: <1385050681-9290-1-git-send-email-roger.pau@citrix.com> <1385050681-9290-3-git-send-email-roger.pau@citrix.com> <21134.21516.508459.132324@mariner.uk.xensource.com> <21134.22006.356685.408733@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VjpAo-0003WU-N5 for xen-devel@lists.xenproject.org; Fri, 22 Nov 2013 11:45:22 +0000 In-Reply-To: <21134.22006.356685.408733@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson , xen-devel@lists.xenproject.org, Ian Campbell List-Id: xen-devel@lists.xenproject.org On 21/11/13 19:50, Ian Jackson wrote: > Ian Jackson writes ("Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend ha= s to close opened files on error"): >> Roger Pau Monne writes ("[PATCH 2/2] libxl: libxl__spawn_qdisk_backend h= as to close opened files on error"): >>> Coverity-ID: 1130517 and 1130518 >>> Signed-off-by: Roger Pau Monn=E9 >> >> 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 > 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 > Coverity-ID: 1130517 and 1130518 > Cc: Roger Pau Monne > Cc: Ian Campbell > --- > 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, li= bxl__dm_spawn_state *dmss) > flexarray_t *dm_args; > char **args; > const char *dm; > - int logfile_w, null, rc; > + int logfile_w =3D -1, null =3D -1, rc; > uint32_t domid =3D dmss->guest_domid; > = > /* Always use qemu-xen as device model */ > @@ -1366,7 +1366,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, li= bxl__dm_spawn_state *dmss) > logfile_w =3D libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u", d= omid)); > if (logfile_w < 0) { > rc =3D logfile_w; > - goto error; > + goto out; > } > null =3D 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 =3D device_model_detached; > rc =3D 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 =3D 0; > = > -error: > - assert(rc); > - dmss->callback(egc, dmss, rc); > + out: > + if (logfile_w >=3D 0) close(logfile_w); > + if (null >=3D 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=E9