[PATCH] libxl: libxl__spawn_qdisk_backend closes fds

[PATCH] libxl: libxl__spawn_qdisk_backend closes fds

[Ian Jackson]

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; the
label "out" 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>
Cc: coverity@xenproject.org
---
 tools/libxl/libxl_dm.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8abed7b..d28ea4d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1392,7 +1392,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 */
@@ -1448,12 +1448,21 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
         libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
     }
 
+    rc = 0;
+
+ 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;
 
 error:
     assert(rc);
-    dmss->callback(egc, dmss, rc);
-    return;
+    goto out;
 }
 
 /* Generic function to signal a Qemu instance to exit */
-- 
1.7.10.4

Re: [PATCH] libxl: libxl__spawn_qdisk_backend closes fds

[Roger Pau Monné]

On 17/03/14 17:32, Ian Jackson wrote:
> 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; the
> label "out" only makes the callback if rc is nonzero.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Coverity-ID: 1130517 and 1130518

There's a similar patch in
<21134.22006.356685.408733@mariner.uk.xensource.com>.

> Cc: Roger Pau Monne <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: coverity@xenproject.org
> ---
>  tools/libxl/libxl_dm.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8abed7b..d28ea4d 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1392,7 +1392,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 */
> @@ -1448,12 +1448,21 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>          libxl__exec(gc, null, logfile_w, logfile_w, dm, args, NULL);
>      }
>  
> +    rc = 0;
> +
> + 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);

Using the device_model_spawn_outcome helper instead of directly calling
the callback will print a nice error message in case spawn has failed.

>      return;
>  
>  error:
>      assert(rc);
> -    dmss->callback(egc, dmss, rc);
> -    return;
> +    goto out;

This jump backwards is kind of strange IMHO, why not just rename the
error label to out and use it instead for both the error and non-error
exit paths?

Roger.

Re: [PATCH] libxl: libxl__spawn_qdisk_backend closes fds

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH] libxl: libxl__spawn_qdisk_backend closes fds"):
> On 17/03/14 17:32, Ian Jackson wrote:
> > 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; the
> > label "out" only makes the callback if rc is nonzero.
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > Coverity-ID: 1130517 and 1130518
> 
> There's a similar patch in
> <21134.22006.356685.408733@mariner.uk.xensource.com>.

Yes.  This posting is an updated version of it.

> > +    /* 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);
> 
> Using the device_model_spawn_outcome helper instead of directly calling
> the callback will print a nice error message in case spawn has failed.

Good idea.  I will do that as a separate patch.

> >      return;
> >  
> >  error:
> >      assert(rc);
> > -    dmss->callback(egc, dmss, rc);
> > -    return;
> > +    goto out;
> 
> This jump backwards is kind of strange IMHO, why not just rename the
> error label to out and use it instead for both the error and non-error
> exit paths?

That would be possible, but would lose the assert(rc).  An error
handling bug elsewhere in the function, which failed to set rc, would
result in it losing the thread of control.  If we don't think the
assert is important I don't mind losing it.

Ian.

Re: [PATCH] libxl: libxl__spawn_qdisk_backend closes fds

[Ian Campbell]

On Tue, 2014-03-18 at 16:23 +0000, Ian Jackson wrote:

> > >      return;
> > >  
> > >  error:
> > >      assert(rc);
> > > -    dmss->callback(egc, dmss, rc);
> > > -    return;
> > > +    goto out;
> > 
> > This jump backwards is kind of strange IMHO, why not just rename the
> > error label to out and use it instead for both the error and non-error
> > exit paths?
> 
> That would be possible, but would lose the assert(rc).  An error
> handling bug elsewhere in the function, which failed to set rc, would
> result in it losing the thread of control.  If we don't think the
> assert is important I don't mind losing it.

The assert is only there to check that we jumped to error on an error,
with a common exit path the assert would be a bit unnecessary I think.

Ian.