Re: [Pkg-xen-devel] xen 4.1 blktap2 support

Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Ian Campbell]

[-- Attachment #1.1: Type: text/plain, Size: 1532 bytes --]

(adding xen-devel) 

On Thu, 2011-04-28 at 13:45 +0200, Niccolò Belli wrote:
> Here is my issue with xl:
> 
> Unknown tapdisk type: tapdisk
> 
> 'tap2:tapdisk:aio:/srv/xen/niko.img,xvda,w',
> 
> blktap is loaded, kernel is 2.6.32-xen from squeeze.

The syntax understood by xl is documented in
docs/misc/xl-disk-configuration.txt. It is intended that it supports all
the useful syntaxes which xend did, although since the xend syntax was
never really specified it is perhaps not complete or correct.

I use 'tap:vhd:/scratch/debian-x86_32-2.vhd,xvda,w' and that works, if
you aren't using vhd I'd expect replacing the 'vhd:' with 'qcow:',
'qcow2:' or 'raw:' etc to work. (nb: xl makes no distinction between tap
and tap2 -- you always get tapdisk2). The 'aio:' is ignored by xl (it's
always on) but you can retain it for compatibility with xend if you
like.

xl-disk-configuration.txt mentions the 'tapdisk:' syntax but I don't see
it actually implemented in xl, it would probably be a nop in any case
(since tap: is sufficient to get the behaviour it specifies), in your
case it would be overridden by the 'aio:' anyway AFAICT. There is
ongoing work to improve xl's handling of the disk cfg syntax.

Ian.

-- 
Ian Campbell

I am currently transitioning to a new OpenPGP key, please see:
http://www.hellion.org.uk/key-transition-2011-04-27-2F6BCD59-to-79074FA8.txt

Reporter (to Mahatma Gandhi): Mr Gandhi, what do you think of Western
	Civilization?
Gandhi:	I think it would be a good idea.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Ian Campbell]

[-- Attachment #1.1: Type: text/plain, Size: 1386 bytes --]

(adding xen-devel)

On Thu, 2011-04-28 at 14:27 +0200, Bastian Blank wrote:
> On Thu, Apr 28, 2011 at 12:28:48PM +0100, Ian Campbell wrote:
> > Perhaps if you would describe what doesn't work for you we could work to
> > fix it, but the above isn't really very helpful, is it?
> 
> The first ones:
> - Silent fail if qemu-dm[1] is missing or failing. It lacks error
>   checking.

Yes, this could certainly be improved.

> - Probably missing close-on-exit flags for several file handlers.

You mean close-on-exec?

the libxl interfaces for exec'ing takes care of closing file handles and
since xl is a one-shot toolstack it generally doesn't have piles of fd's
open. The issue is still worth considering and checking for correctness
though I think, especially within libxc (which has other users than xl).

Do you know of specific instances where the CLOEXEC flag is needed but
missing?

I don't think any of the above qualifies xl as so broken we shouldn't
even suggest people try it, as you started out by saying...

Ian.

> [1]: qemu 0.10 is not supportable in any way security wise.
-- 
Ian Campbell

I am currently transitioning to a new OpenPGP key, please see:
http://www.hellion.org.uk/key-transition-2011-04-27-2F6BCD59-to-79074FA8.txt

/* now make a new head in the exact same spot */
		-- Larry Wall in cons.c from the perl source code

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Bastian Blank]

On Thu, Apr 28, 2011 at 03:20:58PM +0100, Ian Campbell wrote:
> On Thu, 2011-04-28 at 14:27 +0200, Bastian Blank wrote:
> > - Silent fail if qemu-dm[1] is missing or failing. It lacks error
> >   checking.
> Yes, this could certainly be improved.

A hang without any feedback is the worst user-experience possible.

> > - Probably missing close-on-exit flags for several file handlers.
> You mean close-on-exec?

Yes.

> the libxl interfaces for exec'ing takes care of closing file handles and

It tries to, but it can't ever succeed (there is no practical upper
limit for the fd value). Usually this is used to _hide_ other errors.

> Do you know of specific instances where the CLOEXEC flag is needed but
> missing?

Not right now. But because of the hiding properties it is not possible
to detect without changes anyway.

> I don't think any of the above qualifies xl as so broken we shouldn't
> even suggest people try it, as you started out by saying...

Failing without feedback and only running in an environment not
security supportable is broken.

Bastian

-- 
Without followers, evil cannot spread.
		-- Spock, "And The Children Shall Lead", stardate 5029.5

Re: Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Ian Campbell]

On Fri, 2011-04-29 at 12:08 +0100, Bastian Blank wrote:
> On Thu, Apr 28, 2011 at 03:20:58PM +0100, Ian Campbell wrote:
> > On Thu, 2011-04-28 at 14:27 +0200, Bastian Blank wrote:
> > > - Silent fail if qemu-dm[1] is missing or failing. It lacks error
> > >   checking.
> > Yes, this could certainly be improved.
> 
> A hang without any feedback is the worst user-experience possible.

On my systems I get, after ~10s:
libxl: error: libxl_device.c:475:libxl__wait_for_device_model Device Model not ready
xl: fatal error: libxl_create.c:515, rc=-1: libxl__confirm_device_model_startup
libxl: debug: libxl_dm.c:890:libxl__destroy_device_model Device Model already exited

The following makes the obvious qemu-dm not present / not executable
case fail immediately by adding an access(..., X_OK) check, which is
something of an improvement.

This still leaves the delay for other cases, e.g. immediate failure due
to a missing library, bad parameters etc. I think with a little bit of
refactoring libxl__wait_for_device_model() could incorporate the
necessary check for child exit without simply waiting for the entire
delay. I'll take a closer look at this and the other issues you reported
tomorrow.

Ian.

8<------------------------------
libxl: check that device model binary is executable.

This causes us to fail more quickly in more obvious failure case of not
having the right binary installed.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

--- a/tools/libxl/libxl_dm.c	Tue May 03 16:53:22 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Tue May 03 17:34:19 2011 +0100
@@ -762,7 +762,12 @@ int libxl__create_device_model(libxl__gc
         rc = ERROR_FAIL;
         goto out;
     }
-
+    if (access(dm, X_OK) < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                         "device model %s is not executable", dm);
+        rc = ERROR_FAIL;
+        goto out;
+    }
     args = libxl__build_device_model_args(gc, dm, info, disks, num_disks,
                                           vifs, num_vifs);
     if (!args) {


-- 
Ian Campbell
Current Noise: Karma To Burn - Mt. Penetrator

Winning isn't everything, but losing isn't anything.

[PATCH 0 of 4] libxl: improve error handling when device model fails to start early on

[Ian Campbell]

Currently when the device model fails to start there is a delay until
we timeout and report error, some users have misinterpretted this
delay as a hang.

To improve this firstly we can add an access(2) sanity check on the
device model binary before doing anything.

Secondly we can propagate child failures to the parent much quicker by
adding a pipe between the intermediate process (which blocks in
waitpid() on the child until libxl__spawn_detach is called or the
child exits) to allow failure to be reported immediately and with a
slightly more specific error message.

[PATCH 1 of 4] libxl: check that device model binary is executable

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1304499810 -3600
# Node ID 1e3a986b6d298628c6ef8839448860fd41771b2a
# Parent  4d0e906543dc115b5b2f90e0cb44f6affb3f1f99
libxl: check that device model binary is executable.

This causes us to fail more quickly in more obvious failure case of not
having the right binary installed.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 4d0e906543dc -r 1e3a986b6d29 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Tue May 03 16:53:22 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Wed May 04 10:03:30 2011 +0100
@@ -762,7 +762,12 @@ int libxl__create_device_model(libxl__gc
         rc = ERROR_FAIL;
         goto out;
     }
-
+    if (access(dm, X_OK) < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                         "device model %s is not executable", dm);
+        rc = ERROR_FAIL;
+        goto out;
+    }
     args = libxl__build_device_model_args(gc, dm, info, disks, num_disks,
                                           vifs, num_vifs);
     if (!args) {

Re: [PATCH 1 of 4] libxl: check that device model binary is executable

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 1 of 4] libxl: check that device model binary is executable"):
> libxl: check that device model binary is executable.

Thanks, acked and applied all four.

Ian.

[PATCH 2 of 4] libxl: remove redundant call to libxl_domain_device_model

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1304516240 -3600
# Node ID cba88f487f7c5a2d08b0dd5a484c0895c6dee17f
# Parent  1e3a986b6d298628c6ef8839448860fd41771b2a
libxl: remove redundant call to libxl_domain_device_model

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 1e3a986b6d29 -r cba88f487f7c tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Wed May 04 10:03:30 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Wed May 04 14:37:20 2011 +0100
@@ -830,9 +830,7 @@ retry_transaction:
         goto out_close;
     if (!rc) { /* inner child */
         setsid();
-        libxl__exec(null, logfile_w, logfile_w,
-                    libxl__domain_device_model(gc, info),
-                    args);
+        libxl__exec(null, logfile_w, logfile_w, dm, args);
     }
 
     rc = 0;

[PATCH 3 of 4] libxl: pass libxl__spawn_starting to libxl__spawn_spawn

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1304516241 -3600
# Node ID ec008dc43727191f472cd4bd5e59d9d35d9d36c2
# Parent  cba88f487f7c5a2d08b0dd5a484c0895c6dee17f
libxl: pass libxl__spawn_starting to libxl__spawn_spawn.

Passing a libxl__device_model_starting to a generic function and expecting it
to scrobble inside for the generic data structure is a strange interface.
Instead pass in a libxl__spawn_starting and an opaque hook data pointer.

The for_spawn member of libxl__device_model_starting was annotated with
"first!", suggesting that someone intended to use pointer casting tricks to
move between the outer and inner struct. However the field is a pointer not a
inline struct so this doesn't work (and it isn't used this way anyhow). Remove
the comment, and move the field away from the front for good measure.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r cba88f487f7c -r ec008dc43727 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Wed May 04 14:37:20 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Wed May 04 14:37:21 2011 +0100
@@ -825,7 +825,8 @@ retry_transaction:
         }
     }
 
-    rc = libxl__spawn_spawn(gc, p, "device model", dm_xenstore_record_pid);
+    rc = libxl__spawn_spawn(gc, p->for_spawn, "device model",
+                            dm_xenstore_record_pid, p);
     if (rc < 0)
         goto out_close;
     if (!rc) { /* inner child */
diff -r cba88f487f7c -r ec008dc43727 tools/libxl/libxl_exec.c
--- a/tools/libxl/libxl_exec.c	Wed May 04 14:37:20 2011 +0100
+++ b/tools/libxl/libxl_exec.c	Wed May 04 14:37:21 2011 +0100
@@ -92,16 +92,16 @@ void libxl_report_child_exitstatus(libxl
 }
 
 int libxl__spawn_spawn(libxl__gc *gc,
-                      libxl__device_model_starting *starting,
+                      libxl__spawn_starting *for_spawn,
                       const char *what,
                       void (*intermediate_hook)(void *for_spawn,
-                                                pid_t innerchild))
+                                                pid_t innerchild),
+                      void *hook_data)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     pid_t child, got;
     int status;
     pid_t intermediate;
-    libxl__spawn_starting *for_spawn = starting->for_spawn;
 
     if (for_spawn) {
         for_spawn->what = strdup(what);
@@ -127,7 +127,7 @@ int libxl__spawn_spawn(libxl__gc *gc,
     if (!child)
         return 0; /* caller runs child code */
 
-    intermediate_hook(starting, child);
+    intermediate_hook(hook_data, child);
 
     if (!for_spawn) _exit(0); /* just detach then */
 
diff -r cba88f487f7c -r ec008dc43727 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Wed May 04 14:37:20 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Wed May 04 14:37:21 2011 +0100
@@ -237,9 +237,9 @@ typedef struct {
 } libxl__spawn_starting;
 
 typedef struct {
-    libxl__spawn_starting *for_spawn; /* first! */
     char *dom_path; /* from libxl_malloc, only for dm_xenstore_record_pid */
     int domid;
+    libxl__spawn_starting *for_spawn;
 } libxl__device_model_starting;
 
 /* from xl_create */
@@ -277,9 +277,10 @@ _hidden int libxl__wait_for_device_model
                                 void *check_callback_userdata);
 
 _hidden int libxl__spawn_spawn(libxl__gc *gc,
-                      libxl__device_model_starting *starting,
+                      libxl__spawn_starting *starting,
                       const char *what,
-                      void (*intermediate_hook)(void *for_spawn, pid_t innerchild));
+                      void (*intermediate_hook)(void *for_spawn, pid_t innerchild),
+                      void *hook_data);
 _hidden int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid);
 
   /* Logs errors.  A copy of "what" is taken.  Return values:

[PATCH 4 of 4] libxl: add statup checks to libxl__wait_for_device_model

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1304516515 -3600
# Node ID 36871b37a9bd286d6ef291b41513c33325db9719
# Parent  ec008dc43727191f472cd4bd5e59d9d35d9d36c2
libxl: add statup checks to libxl__wait_for_device_model

When the device model is starting up push checks for spawn failure down into
libxl__wait_for_device_model, allowing us to fail more quickly when the device
model fails to start (e.g. due to a missing library or an early setup error
etc).

In order to allow the select loop in libxl__wait_for_device_model to wake when
the child dies add pipe between the parent and the intermediate process which
the intermediate process can use to signal the parent.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed May 04 14:37:21 2011 +0100
+++ b/tools/libxl/libxl.c	Wed May 04 14:41:55 2011 +0100
@@ -522,7 +522,7 @@ int libxl_domain_unpause(libxl_ctx *ctx,
         state = libxl__xs_read(&gc, XBT_NULL, path);
         if (state != NULL && !strcmp(state, "paused")) {
             libxl__xs_write(&gc, XBT_NULL, libxl__sprintf(&gc, "/local/domain/0/device-model/%d/command", domid), "continue");
-            libxl__wait_for_device_model(&gc, domid, "running", NULL, NULL);
+            libxl__wait_for_device_model(&gc, domid, "running", NULL, NULL, NULL);
         }
     }
     ret = xc_domain_unpause(ctx->xch, domid);
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Wed May 04 14:37:21 2011 +0100
+++ b/tools/libxl/libxl_device.c	Wed May 04 14:41:55 2011 +0100
@@ -410,6 +410,7 @@ out:
 
 int libxl__wait_for_device_model(libxl__gc *gc,
                                  uint32_t domid, char *state,
+                                 libxl__device_model_starting *starting,
                                  int (*check_callback)(libxl__gc *gc,
                                                        uint32_t domid,
                                                        const char *state,
@@ -439,7 +440,17 @@ int libxl__wait_for_device_model(libxl__
     tv.tv_sec = LIBXL_DEVICE_MODEL_START_TIMEOUT;
     tv.tv_usec = 0;
     nfds = xs_fileno(xsh) + 1;
+    if (starting && starting->for_spawn->fd > xs_fileno(xsh))
+        nfds = starting->for_spawn->fd + 1;
+
     while (rc > 0 || (!rc && tv.tv_sec > 0)) {
+        if ( starting ) {
+            rc = libxl__spawn_check(gc, starting->for_spawn);
+            if ( rc ) {
+                rc = -1;
+                goto err_died;
+            }
+        }
         p = xs_read(xsh, XBT_NULL, path, &len);
         if ( NULL == p )
             goto again;
@@ -461,15 +472,24 @@ again:
         free(p);
         FD_ZERO(&rfds);
         FD_SET(xs_fileno(xsh), &rfds);
+        if (starting)
+            FD_SET(starting->for_spawn->fd, &rfds);
         rc = select(nfds, &rfds, NULL, NULL, &tv);
         if (rc > 0) {
-            l = xs_read_watch(xsh, &num);
-            if (l != NULL)
-                free(l);
-            else
-                goto again;
+            if (FD_ISSET(xs_fileno(xsh), &rfds)) {
+                l = xs_read_watch(xsh, &num);
+                if (l != NULL)
+                    free(l);
+                else
+                    goto again;
+            }
+            if (starting && FD_ISSET(starting->for_spawn->fd, &rfds)) {
+                unsigned char dummy;
+                read(starting->for_spawn->fd, &dummy, sizeof(dummy));
+            }
         }
     }
+err_died:
     xs_unwatch(xsh, path, path);
     xs_daemon_close(xsh);
     LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Device Model not ready");
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Wed May 04 14:37:21 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Wed May 04 14:41:55 2011 +0100
@@ -855,14 +855,12 @@ static int detach_device_model(libxl__gc
     return rc;
 }
 
-
 int libxl__confirm_device_model_startup(libxl__gc *gc,
                                        libxl__device_model_starting *starting)
 {
-    int problem = libxl__wait_for_device_model(gc, starting->domid, "running", NULL, NULL);
     int detach;
-    if ( !problem )
-        problem = libxl__spawn_check(gc, starting->for_spawn);
+    int problem = libxl__wait_for_device_model(gc, starting->domid, "running",
+                                               starting, NULL, NULL);
     detach = detach_device_model(gc, starting);
     return problem ? problem : detach;
 }
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Wed May 04 14:37:21 2011 +0100
+++ b/tools/libxl/libxl_dom.c	Wed May 04 14:41:55 2011 +0100
@@ -543,7 +543,7 @@ int libxl__domain_save_device_model(libx
 
     LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Saving device model state to %s", filename);
     libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "/local/domain/0/device-model/%d/command", domid), "save");
-    libxl__wait_for_device_model(gc, domid, "paused", NULL, NULL);
+    libxl__wait_for_device_model(gc, domid, "paused", NULL, NULL, NULL);
 
     if (stat(filename, &st) < 0)
     {
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_exec.c
--- a/tools/libxl/libxl_exec.c	Wed May 04 14:37:21 2011 +0100
+++ b/tools/libxl/libxl_exec.c	Wed May 04 14:41:55 2011 +0100
@@ -26,6 +26,7 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <signal.h> /* for SIGKILL */
+#include <fcntl.h>
 
 #include "libxl.h"
 #include "libxl_internal.h"
@@ -91,6 +92,22 @@ void libxl_report_child_exitstatus(libxl
     }
 }
 
+static int libxl__set_fd_flag(libxl__gc *gc, int fd, int flag)
+{
+    int flags;
+
+    flags = fcntl(fd, F_GETFL);
+    if (flags == -1)
+        return ERROR_FAIL;
+
+    flags |= flag;
+
+    if (fcntl(fd, F_SETFL, flags) == -1)
+        return ERROR_FAIL;
+
+    return 0;
+}
+
 int libxl__spawn_spawn(libxl__gc *gc,
                       libxl__spawn_starting *for_spawn,
                       const char *what,
@@ -100,22 +117,33 @@ int libxl__spawn_spawn(libxl__gc *gc,
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     pid_t child, got;
-    int status;
+    int status, rc;
     pid_t intermediate;
+    int pipes[2];
+    unsigned char dummy = 0;
 
     if (for_spawn) {
         for_spawn->what = strdup(what);
         if (!for_spawn->what) return ERROR_NOMEM;
+
+        if (libxl_pipe(ctx, pipes) < 0)
+            goto err_parent;
+        if (libxl__set_fd_flag(gc, pipes[0], O_NONBLOCK) < 0 ||
+            libxl__set_fd_flag(gc, pipes[1], O_NONBLOCK) < 0)
+            goto err_parent_pipes;
     }
 
     intermediate = libxl_fork(ctx);
-    if (intermediate ==-1) {
-        if (for_spawn) free(for_spawn->what);
-        return ERROR_FAIL;
-    }
+    if (intermediate ==-1)
+        goto err_parent_pipes;
+
     if (intermediate) {
         /* parent */
-        if (for_spawn) for_spawn->intermediate = intermediate;
+        if (for_spawn) {
+            for_spawn->intermediate = intermediate;
+            for_spawn->fd = pipes[0];
+            close(pipes[1]);
+        }
         return 1;
     }
 
@@ -124,8 +152,10 @@ int libxl__spawn_spawn(libxl__gc *gc,
     child = fork();
     if (child == -1)
         exit(255);
-    if (!child)
+    if (!child) {
+        if (for_spawn) close(pipes[1]);
         return 0; /* caller runs child code */
+    }
 
     intermediate_hook(hook_data, child);
 
@@ -134,9 +164,23 @@ int libxl__spawn_spawn(libxl__gc *gc,
     got = call_waitpid(ctx->waitpid_instead, child, &status, 0);
     assert(got == child);
 
-    _exit(WIFEXITED(status) ? WEXITSTATUS(status) :
+    rc = (WIFEXITED(status) ? WEXITSTATUS(status) :
           WIFSIGNALED(status) && WTERMSIG(status) < 127
           ? WTERMSIG(status)+128 : -1);
+    if (for_spawn)
+        write(pipes[1], &dummy, sizeof(dummy));
+    _exit(rc);
+
+ err_parent_pipes:
+    if (for_spawn) {
+        close(pipes[0]);
+        close(pipes[1]);
+    }
+
+ err_parent:
+    if (for_spawn) free(for_spawn->what);
+
+    return ERROR_FAIL;
 }
 
 static void report_spawn_intermediate_status(libxl__gc *gc,
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Wed May 04 14:37:21 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Wed May 04 14:41:55 2011 +0100
@@ -233,6 +233,7 @@ typedef struct {
     /* put this in your own status structure as returned to application */
     /* all fields are private to libxl_spawn_... */
     pid_t intermediate;
+    int fd;
     char *what; /* malloc'd in spawn_spawn */
 } libxl__spawn_starting;
 
@@ -270,6 +271,7 @@ _hidden int libxl__confirm_device_model_
 _hidden int libxl__detach_device_model(libxl__gc *gc, libxl__device_model_starting *starting);
 _hidden int libxl__wait_for_device_model(libxl__gc *gc,
                                 uint32_t domid, char *state,
+                                libxl__device_model_starting *starting,
                                 int (*check_callback)(libxl__gc *gc,
                                                       uint32_t domid,
                                                       const char *state,
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Wed May 04 14:37:21 2011 +0100
+++ b/tools/libxl/libxl_pci.c	Wed May 04 14:41:55 2011 +0100
@@ -612,7 +612,8 @@ static int do_pci_add(libxl__gc *gc, uin
 
     hvm = libxl__domain_is_hvm(gc, domid);
     if (hvm) {
-        if (libxl__wait_for_device_model(gc, domid, "running", NULL, NULL) < 0) {
+        if (libxl__wait_for_device_model(gc, domid, "running",
+                                         NULL, NULL, NULL) < 0) {
             return ERROR_FAIL;
         }
         path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
@@ -626,7 +627,8 @@ static int do_pci_add(libxl__gc *gc, uin
                            pcidev->bus, pcidev->dev, pcidev->func);
         path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/command", domid);
         xs_write(ctx->xsh, XBT_NULL, path, "pci-ins", strlen("pci-ins"));
-        rc = libxl__wait_for_device_model(gc, domid, NULL, pci_ins_check, state);
+        rc = libxl__wait_for_device_model(gc, domid, NULL, NULL,
+                                          pci_ins_check, state);
         path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/parameter", domid);
         vdevfn = libxl__xs_read(gc, XBT_NULL, path);
         path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
@@ -843,7 +845,8 @@ static int do_pci_remove(libxl__gc *gc, 
 
     hvm = libxl__domain_is_hvm(gc, domid);
     if (hvm) {
-        if (libxl__wait_for_device_model(gc, domid, "running", NULL, NULL) < 0) {
+        if (libxl__wait_for_device_model(gc, domid, "running",
+                                         NULL, NULL, NULL) < 0) {
             return ERROR_FAIL;
         }
         path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
@@ -857,7 +860,8 @@ static int do_pci_remove(libxl__gc *gc, 
          * device-model for function 0 */
         if ( !force && (pcidev->vdevfn & 0x7) == 0 ) {
             xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));
-            if (libxl__wait_for_device_model(gc, domid, "pci-removed", NULL, NULL) < 0) {
+            if (libxl__wait_for_device_model(gc, domid, "pci-removed",
+                                             NULL, NULL, NULL) < 0) {
                 LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Device Model didn't respond in time");
                 /* This depends on guest operating system acknowledging the
                  * SCI, if it doesn't respond in time then we may wish to 

Re: [PATCH 4 of 4] libxl: add statup checks to libxl__wait_for_device_model

[Ian Campbell]

On Wed, 2011-05-04 at 15:51 +0100, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1304516515 -3600
> # Node ID 36871b37a9bd286d6ef291b41513c33325db9719
> # Parent  ec008dc43727191f472cd4bd5e59d9d35d9d36c2
> libxl: add statup checks to libxl__wait_for_device_model
> 
> When the device model is starting up push checks for spawn failure down into
> libxl__wait_for_device_model, allowing us to fail more quickly when the device
> model fails to start (e.g. due to a missing library or an early setup error
> etc).
> 
> In order to allow the select loop in libxl__wait_for_device_model to wake when
> the child dies add pipe between the parent and the intermediate process which
> the intermediate process can use to signal the parent.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

I accidentally removed the close of the read side of the pipes from the
intermediate process while refactoring. So this incremental fix is
needed:

--- a/tools/libxl/libxl_exec.c	Wed May 04 17:03:14 2011 +0100
+++ b/tools/libxl/libxl_exec.c	Wed May 04 17:22:05 2011 +0100
@@ -144,6 +144,7 @@ int libxl__spawn_spawn(libxl__gc *gc,
     }
 
     /* we are now the intermediate process */
+    if (for_spawn) close(pipes[0]);
 
     child = fork();
     if (child == -1)

Re: [PATCH 4 of 4] libxl: add statup checks to libxl__wait_for_device_model

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 4 of 4] libxl: add statup checks to libxl__wait_for_device_model"):
> I accidentally removed the close of the read side of the pipes from the
> intermediate process while refactoring. So this incremental fix is
> needed:

This hunk was in your O_CLOEXEC patch so is now in the tree, although
not part of the changeset for libxl startup checks.

Ian.

> --- a/tools/libxl/libxl_exec.c	Wed May 04 17:03:14 2011 +0100
> +++ b/tools/libxl/libxl_exec.c	Wed May 04 17:22:05 2011 +0100
> @@ -144,6 +144,7 @@ int libxl__spawn_spawn(libxl__gc *gc,
>      }
>  
>      /* we are now the intermediate process */
> +    if (for_spawn) close(pipes[0]);
>  
>      child = fork();
>      if (child == -1)

Re: [PATCH 4 of 4] libxl: add statup checks to libxl__wait_for_device_model

[Ian Campbell]

On Tue, 2011-05-24 at 16:57 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 4 of 4] libxl: add statup checks to libxl__wait_for_device_model"):
> > I accidentally removed the close of the read side of the pipes from the
> > intermediate process while refactoring. So this incremental fix is
> > needed:
> 
> This hunk was in your O_CLOEXEC patch so is now in the tree, although
> not part of the changeset for libxl startup checks.

Either way is fine with me.

Thanks,
Ian.

> 
> Ian.
> 
> > --- a/tools/libxl/libxl_exec.c	Wed May 04 17:03:14 2011 +0100
> > +++ b/tools/libxl/libxl_exec.c	Wed May 04 17:22:05 2011 +0100
> > @@ -144,6 +144,7 @@ int libxl__spawn_spawn(libxl__gc *gc,
> >      }
> >  
> >      /* we are now the intermediate process */
> > +    if (for_spawn) close(pipes[0]);
> >  
> >      child = fork();
> >      if (child == -1)

Re: [PATCH 0 of 4] libxl: improve error handling when device model fails to start early on

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 0 of 4] libxl: improve error handling when device model fails to start early on"):
> Currently when the device model fails to start there is a delay until
> we timeout and report error, some users have misinterpretted this
> delay as a hang.

This is disappointing.  I fixed exactly this problem about 6 months
ago.

> Secondly we can propagate child failures to the parent much quicker by
> adding a pipe between the intermediate process (which blocks in
> waitpid() on the child until libxl__spawn_detach is called or the
> child exits) to allow failure to be reported immediately and with a
> slightly more specific error message.

And this is what I thought I'd done.

Ian.

Re: [PATCH 0 of 4] libxl: improve error handling when device model fails to start early on

[Ian Campbell]

On Thu, 2011-05-19 at 17:04 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH 0 of 4] libxl: improve error handling when device model fails to start early on"):
> > Currently when the device model fails to start there is a delay until
> > we timeout and report error, some users have misinterpretted this
> > delay as a hang.
> 
> This is disappointing.  I fixed exactly this problem about 6 months
> ago.

Hmm, now you mention it that does sound familiar. And indeed:

        changeset:   20468:2f7cb671ef38
        user:        Keir Fraser <keir.fraser@citrix.com>
        date:        Mon Nov 23 07:01:51 2009 +0000
        description:
        libxenlight: check for early failures of qemu-dm
        
        This patch makes xl create check whether qemu-dm has started
        correctly, and causes it to fail immediately with appropriate errors
        if not.  There are other bugfixes too.
        [...]
        Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

So more like 18 months ago? (or I found another different attempt ;-))

> > Secondly we can propagate child failures to the parent much quicker by
> > adding a pipe between the intermediate process (which blocks in
> > waitpid() on the child until libxl__spawn_detach is called or the
> > child exits) to allow failure to be reported immediately and with a
> > slightly more specific error message.
> 
> And this is what I thought I'd done.

Either way, it appears to be broken now.

You can confirm by removing one of the libraries that qemu-dm needs, or
by chmod -x.

Ian.

[PATCH] Re: Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Ian Campbell]

On Fri, 2011-04-29 at 12:08 +0100, Bastian Blank wrote:
> On Thu, Apr 28, 2011 at 03:20:58PM +0100, Ian Campbell wrote:
> > On Thu, 2011-04-28 at 14:27 +0200, Bastian Blank wrote:

> > > - Probably missing close-on-exit flags for several file handlers.
> > You mean close-on-exec?
> 
> Yes.
> 
> > the libxl interfaces for exec'ing takes care of closing file handles and
> 
> It tries to, but it can't ever succeed (there is no practical upper
> limit for the fd value). Usually this is used to _hide_ other errors.

Agreed, and anyway libxl_exec is a generic is supposed to be a generic
interface so having it close file handles which the caller _wants_ to
pass to the child is a bit unhelpful.

Lets try the below to start with.

Ian.

8<-----------------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1304597796 -3600
# Node ID 158e845f0d2cc1d263da4ee5ea725a0d90a0e42d
# Parent  117680fafe0c901171e9d519be37072ed0b8a765
libxl: don't close file descriptors 4..255 in libxl__exec

It prevents callers from deliberately passing file descriptors to the child,
hides any callers who erroneously do so and doesn't deal with all file
descriptors in any case.

Rather than remove it all together replace it with some debug code
which checks for open file handles which do not have either O_CLOEXEC
or FD_CLOEXEC set. To enable the debug set _LIBXL_DEBUG_EXEC_FDS=1 to
print any open and non-CLOEXEC non-stdio FDs just before libxl__exec
actualy calls exec. Set _LIBXL_DEBUG_EXEC_FDS=2 to abort if any of
these exist.

On the basis of this debugging fix some leaked filehandles:
  * The read end of the pipe used to wake the parent from the
    intermediate process during libxl__spawn_spawn was leaked into the
    intermediate process.
  * The file descriptor representing the xl lock was not marked
    O_CLOEXEC (the lock itself is already specified to not be
    inherited across a fork).
  * The file descriptors passed to libxl__exec to be dup'd as
    std{in,out,err} were leaked at their original number. They are
    harmless (an attacker can just as easily use fd 0..2) but close
    anyway since it removes a case which a person evaluating open fd's
    needs to consider.
  * libxl_run_bootloader was leaking the xenconsole pty master into
    the bootloader child process.
  * If the bootloader fails to get as far as opening its end of the
    FIFO then we can also hang, check that the process has not exited
    as part of that loop. (we actually block opening the FIFO too so
    this is only a partial fix for the case where the bootlader has
    crashed quickly).

With these fixes I have tested that device models, bootloaders
(pygrub) and vncviewers which are spawned via libxl__exec with no
unexpected file descriptors open, at least in my configuration.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 117680fafe0c -r 158e845f0d2c tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Wed May 04 17:03:14 2011 +0100
+++ b/tools/libxl/libxl_bootloader.c	Thu May 05 13:16:36 2011 +0100
@@ -115,6 +115,7 @@ static int open_xenconsoled_pty(int *mas
 #endif
 
     fcntl(*master, F_SETFL, O_NDELAY);
+    fcntl(*master, F_SETFD, FD_CLOEXEC);
 
     return 0;
 }
@@ -393,6 +394,9 @@ int libxl_run_bootloader(libxl_ctx *ctx,
     }
 
     while (1) {
+        if (waitpid(pid, &blrc, WNOHANG) == pid)
+            goto out_close;
+
         fifo_fd = open(fifo, O_RDONLY);
         if (fifo_fd > -1)
             break;
diff -r 117680fafe0c -r 158e845f0d2c tools/libxl/libxl_exec.c
--- a/tools/libxl/libxl_exec.c	Wed May 04 17:03:14 2011 +0100
+++ b/tools/libxl/libxl_exec.c	Thu May 05 13:16:36 2011 +0100
@@ -36,20 +36,72 @@ static int call_waitpid(pid_t (*waitpid_
     return (waitpid_cb) ? waitpid_cb(pid, status, options) : waitpid(pid, status, options);
 }
 
+static void check_open_fds(const char *what)
+{
+    const char *env_debug;
+    int debug;
+    int i, flags, badness = 0;
+    char path[PATH_MAX];
+    char link[PATH_MAX+1];
+
+    env_debug = getenv("_LIBXL_DEBUG_EXEC_FDS");
+    if (!env_debug) return;
+
+    debug = strtol(env_debug, (char **) NULL, 10);atoi(env_debug);
+    if (debug <= 0) return;
+
+    for (i = 4; i < 256; i++) {
+#ifdef __linux__
+        size_t len;
+#endif
+        flags = fcntl(i, F_GETFD);
+        if ( flags == -1 ) {
+            if ( errno != EBADF )
+                fprintf(stderr, "libxl: execing %s: fd %d flags returned %s (%d)\n",
+                        what, i, strerror(errno), errno);
+            continue;
+        }
+
+        if ( flags & FD_CLOEXEC )
+            continue;
+
+        badness++;
+
+#ifdef __linux__
+        snprintf(path, PATH_MAX, "/proc/%d/fd/%d", getpid(), i);
+        len = readlink(path, link, PATH_MAX);
+        if (len > 0) {
+            link[len] = '\0';
+            fprintf(stderr, "libxl: execing %s: fd %d is open to %s with flags %#x\n",
+                    what, i, link, flags);
+        } else
+#endif
+            fprintf(stderr, "libxl: execing %s: fd %d is open with flags %#x\n",
+                    what, i, flags);
+    }
+    if (debug < 2) return;
+    if (badness) abort();
+}
+
 void libxl__exec(int stdinfd, int stdoutfd, int stderrfd, const char *arg0,
                 char **args)
      /* call this in the child */
 {
-    int i;
-
     if (stdinfd != -1)
         dup2(stdinfd, STDIN_FILENO);
     if (stdoutfd != -1)
         dup2(stdoutfd, STDOUT_FILENO);
     if (stderrfd != -1)
         dup2(stderrfd, STDERR_FILENO);
-    for (i = 4; i < 256; i++)
-        close(i);
+
+    if (stdinfd != -1)
+        close(stdinfd);
+    if (stdoutfd != -1 && stdoutfd != stdinfd)
+        close(stdoutfd);
+    if (stderrfd != -1 && stderrfd != stdinfd && stderrfd != stdoutfd)
+        close(stderrfd);
+
+    check_open_fds(arg0);
 
     signal(SIGPIPE, SIG_DFL);
     /* in case our caller set it to IGN.  subprocesses are entitled
@@ -148,6 +200,7 @@ int libxl__spawn_spawn(libxl__gc *gc,
     }
 
     /* we are now the intermediate process */
+    if (for_spawn) close(pipes[0]);
 
     child = fork();
     if (child == -1)
diff -r 117680fafe0c -r 158e845f0d2c tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed May 04 17:03:14 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu May 05 13:16:36 2011 +0100
@@ -199,7 +199,7 @@ static int acquire_lock(void)
     fl.l_whence = SEEK_SET;
     fl.l_start = 0;
     fl.l_len = 0;
-    fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR);
+    fd_lock = open(lockfile, O_WRONLY|O_CREAT|O_CLOEXEC, S_IWUSR);
     if (fd_lock < 0) {
         fprintf(stderr, "cannot open the lockfile %s errno=%d\n", lockfile, errno);
         return ERROR_FAIL;


-- 
Ian Campbell

Is a person who blows up banks an econoclast?

[PATCH] Re: Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Ian Jackson]

Ian Campbell writes ("[PATCH] Re: [Xen-devel] Re: [Pkg-xen-devel] xen 4.1 blktap2 support"):
> Agreed, and anyway libxl_exec is a generic is supposed to be a generic
> interface so having it close file handles which the caller _wants_ to
> pass to the child is a bit unhelpful.
> 
> Lets try the below to start with.

It looks good but you are missing some #includes I think:

cc1: warnings being treated as errors
libxl_exec.c: In function 'check_open_fds':
libxl_exec.c:56: error: implicit declaration of function 'fcntl'
libxl_exec.c:56: error: 'F_GETFD' undeclared (first use in this function)
libxl_exec.c:56: error: (Each undeclared identifier is reported only once
libxl_exec.c:56: error: for each function it appears in.)
libxl_exec.c:64: error: 'FD_CLOEXEC' undeclared (first use in this function)
libxl_exec.c: In function 'libxl__spawn_spawn':
libxl_exec.c:175: error: 'pipes' undeclared (first use in this function)
make: *** [libxl_exec.o] Error 1

Ian.

Re: [PATCH] Re: Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Ian Campbell]

On Fri, 2011-05-20 at 18:08 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH] Re: [Xen-devel] Re: [Pkg-xen-devel] xen 4.1 blktap2 support"):
> > Agreed, and anyway libxl_exec is a generic is supposed to be a generic
> > interface so having it close file handles which the caller _wants_ to
> > pass to the child is a bit unhelpful.
> > 
> > Lets try the below to start with.
> 
> It looks good but you are missing some #includes I think:
> 
> cc1: warnings being treated as errors
> libxl_exec.c: In function 'check_open_fds':
> libxl_exec.c:56: error: implicit declaration of function 'fcntl'
> libxl_exec.c:56: error: 'F_GETFD' undeclared (first use in this function)
> libxl_exec.c:56: error: (Each undeclared identifier is reported only once
> libxl_exec.c:56: error: for each function it appears in.)
> libxl_exec.c:64: error: 'FD_CLOEXEC' undeclared (first use in this function)

fcntl(2) says fcntl needs unistd.h (already present in source) and
fcntl.h (added by this patch). Did a hunk get rejected? I'm build
testing on Squeeze.

> libxl_exec.c: In function 'libxl__spawn_spawn':
> libxl_exec.c:175: error: 'pipes' undeclared (first use in this function)
> make: *** [libxl_exec.o] Error 1

pipes is a local variable added by this patch which should have been
reference several times by line 175... Another .rej?

Ian.
-- 
Ian Campbell

The world really isn't any worse.  It's just that the news coverage
is so much better.

Re: [PATCH] Re: Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] Re: [Xen-devel] Re: [Pkg-xen-devel] xen 4.1 blktap2 support"):
> pipes is a local variable added by this patch which should have been
> reference several times by line 175... Another .rej?

I'm pretty sure there weren't any rejects, but perhaps I was applying
them to the wrong tree.  IIRC I was moonlighting in a conference
programme at the time.

Anyway, it applies and builds fine today.  Sorry for the noise.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Bastian Blank]

On Thu, Apr 28, 2011 at 02:27:19PM +0200, Bastian Blank wrote:
> The first ones:
- It defaults to use qemu not when it is needed but when blktap is not
  loaded.

Bastian

-- 
The best diplomat I know is a fully activated phaser bank.
		-- Scotty

Re: [Pkg-xen-devel] xen 4.1 blktap2 support

[Bastian Blank]

On Sat, Apr 30, 2011 at 03:09:44PM +0200, Bastian Blank wrote:
> On Thu, Apr 28, 2011 at 02:27:19PM +0200, Bastian Blank wrote:
> > The first ones:
> - It defaults to use qemu not when it is needed but when blktap is not
>   loaded.

Okay, fixed in xen-unstable. I added a backport to the package.

Bastian

-- 
We Klingons believe as you do -- the sick should die.  Only the strong
should live.
		-- Kras, "Friday's Child", stardate 3497.2