Xen devel - Nov 2013 - [PATCH 0/2] libxl/xl: two more coverity related fixes

The first patch is a leftover from the switch to 
libxl__create_qemu_logfile, and while there it also handles possible 
errors when opening /dev/null.

The second one is a fix for the issues present in do_daemonize.

Thanks, Roger.

Checking the logfile_w fd for -1 on failure is no longer true, because
libxl__create_qemu_logfile will now return ERROR_FAIL on failure which
is -3.

While there also add an error check for opening /dev/null.

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>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxl/libxl_dm.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 469b82e..017ef70 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1228,6 +1228,11 @@ void libxl__spawn_local_dm(libxl__egc *egc,
libxl__dm_spawn_state *dmss)
         goto out;
     }
     null = open("/dev/null", O_RDONLY);
+    if (null < 0) {
+        LOGE(ERROR, "unable to open /dev/null");
+        rc = ERROR_FAIL;
+        goto out_close;
+    }
 
     const char *dom_path = libxl__xs_get_dompath(gc, domid);
     spawn->pidpath = GCSPRINTF("%s/%s", dom_path,
"image/device-model-pid");
@@ -1275,8 +1280,8 @@ retry_transaction:
     rc = 0;
 
 out_close:
-    if (null != -1) close(null);
-    if (logfile_w != -1) close(logfile_w);
+    if (null >= 0) close(null);
+    if (logfile_w >= 0) close(logfile_w);
 out:
     if (rc)
         device_model_spawn_outcome(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

Fix usage of CHK_ERRNO in do_daemonize and also remove the usage of a
bogus for(;;).

Coverity-ID: 1130516 and 1130520
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>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 341863e..0f623a0 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -411,14 +411,14 @@ static int do_daemonize(char *name)
 
     child1 = xl_fork(child_waitdaemon);
     if (child1) {
-        for (;;) {
-            got_child = xl_waitpid(child_waitdaemon, &status, 0);
-            if (got_child == child1) break;
+        got_child = xl_waitpid(child_waitdaemon, &status, 0);
+        if (got_child != child1) {
             assert(got_child == -1);
-            perror("failed to wait for daemonizing child");
+            LOG("failed to wait for daemonizing child: %s",
strerror(errno));
             ret = ERROR_FAIL;
             goto out;
         }
+
         if (status) {
             libxl_report_child_exitstatus(ctx, XTL_ERROR,
                        "daemonizing child", child1, status);
@@ -437,16 +437,15 @@ static int do_daemonize(char *name)
         exit(-1);
     }
 
-    CHK_ERRNO(( logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND,
-                               0644) )<0);
+    CHK_ERRNO(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644));
     free(fullname);
 
-    CHK_ERRNO(( nullfd = open("/dev/null", O_RDONLY) )<0);
+    CHK_ERRNO(nullfd = open("/dev/null", O_RDONLY));
     dup2(nullfd, 0);
     dup2(logfile, 1);
     dup2(logfile, 2);
 
-    CHK_ERRNO(daemon(0, 1) < 0);
+    CHK_ERRNO(daemon(0, 1));
 
 out:
     return ret;
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 22/11/13 11:54, Roger Pau Monne wrote:
> Fix usage of CHK_ERRNO in do_daemonize and also remove the usage of a
> bogus for(;;).
>
> Coverity-ID: 1130516 and 1130520
> 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>
Looks plausibly like it will fix the identified issues.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   15 +++++++--------
>  1 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 341863e..0f623a0 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -411,14 +411,14 @@ static int do_daemonize(char *name)
>  
>      child1 = xl_fork(child_waitdaemon);
>      if (child1) {
> -        for (;;) {
> -            got_child = xl_waitpid(child_waitdaemon, &status, 0);
> -            if (got_child == child1) break;
> +        got_child = xl_waitpid(child_waitdaemon, &status, 0);
> +        if (got_child != child1) {
>              assert(got_child == -1);
> -            perror("failed to wait for daemonizing child");
> +            LOG("failed to wait for daemonizing child: %s",
strerror(errno));
>              ret = ERROR_FAIL;
>              goto out;
>          }
> +
>          if (status) {
>              libxl_report_child_exitstatus(ctx, XTL_ERROR,
>                         "daemonizing child", child1, status);
> @@ -437,16 +437,15 @@ static int do_daemonize(char *name)
>          exit(-1);
>      }
>  
> -    CHK_ERRNO(( logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND,
> -                               0644) )<0);
> +    CHK_ERRNO(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644));
>      free(fullname);
>  
> -    CHK_ERRNO(( nullfd = open("/dev/null", O_RDONLY) )<0);
> +    CHK_ERRNO(nullfd = open("/dev/null", O_RDONLY));
>      dup2(nullfd, 0);
>      dup2(logfile, 1);
>      dup2(logfile, 2);
>  
> -    CHK_ERRNO(daemon(0, 1) < 0);
> +    CHK_ERRNO(daemon(0, 1));
>  
>  out:
>      return ret;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Fri, 2013-11-22 at 12:54 +0100, Roger Pau Monne
wrote:
> The first patch is a leftover from the switch to 
> libxl__create_qemu_logfile, and while there it also handles possible 
> errors when opening /dev/null.
> 
> The second one is a fix for the issues present in do_daemonize.
BOth acked + applied. thanks.