[PATCH 0/2] libxl: fixes for coverity reported issues

[PATCH 0/2] libxl: fixes for coverity reported issues

[Roger Pau Monne]

Fixes for two bugs coverity found after the driver domain series went 
in.

[PATCH 1/2] libxl: fix use of aodev->dev after free

[Roger Pau Monne]

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

[PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Roger Pau Monne]

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

Re: [PATCH 0/2] libxl: fixes for coverity reported issues

[Andrew Cooper]

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

Re: [PATCH 1/2] libxl: fix use of aodev->dev after free

[Ian Jackson]

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.

Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Ian Jackson]

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

Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Ian Jackson]

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

Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Andrew Cooper]

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;
>  }
>  

Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Andrew Cooper]

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

Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Roger Pau Monné]

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>

Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Ian Campbell]

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

Re: [PATCH 2/2] libxl: libxl__spawn_qdisk_backend has to close opened files on error

[Ian Campbell]

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