From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error Date: Thu, 21 Nov 2013 19:07:01 +0000 Message-ID: <528E59D5.5070404@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> <528E5938.8020204@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VjZaj-0001q6-FY for xen-devel@lists.xenproject.org; Thu, 21 Nov 2013 19:07:05 +0000 In-Reply-To: <528E5938.8020204@citrix.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 Cc: xen-devel@lists.xenproject.org, Ian Campbell , Roger Pau Monne List-Id: xen-devel@lists.xenproject.org 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 h= as 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=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 > 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, l= ibxl__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; > The rc logic is a little awkward. Would it be better to initialise to > -1 here... > >> 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, l= ibxl__dm_spawn_state *dmss) >> logfile_w =3D libxl__create_qemu_logfile(gc, GCSPRINTF("qdisk-%u", = domid)); >> if (logfile_w < 0) { >> rc =3D logfile_w; > ... and avoid this somewhat odd assignment? > > ~Andrew > >> - goto error; >> + goto out; >> } >> null =3D 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 =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); >> return; >> } >> = > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel