Skip to contentSkip to conversation

xen 4.1 blktap2 support

Pkg-xen-devel In review Ian Campbell 5 participants 21 messages Apr 28, 2011 – May 24, 2011 10 messages not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

#9 lore raw

(adding xen-devel)

Quoted 7 lines · Niccolò Belli

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.

Signature · 8 lines
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.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

A message that is not in the archive (sent off-list or not yet received): [email protected]

#11 lore raw

(adding xen-devel)

Quoted 7 lines · Bastian Blank
Quoted 2 lines · Ian Campbell

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.

Quoted 1 line

- 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.

Quoted 1 line

[1]: qemu 0.10 is not supportable in any way security wise.

Signature · 7 lines
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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#12 lore raw
Quoted 4 lines · Ian Campbell
Quoted 2 lines · Bastian Blank

- 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.

Quoted 2 lines
Quoted 1 line

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

You mean close-on-exec?

Yes.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 2 lines

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

Signature · 2 lines
Without followers, evil cannot spread.
		-- Spock, "And The Children Shall Lead", stardate 5029.5
#13 lore raw
Quoted 7 lines · Bastian Blank
Quoted 4 lines · Ian Campbell
Quoted 2 lines · Bastian Blank

- 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.

tools/libxl/libxl_dm.c +6 -1
@@ -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) {
Signature · 4 lines
Ian Campbell
Current Noise: Karma To Burn - Mt. Penetrator

Winning isn't everything, but losing isn't anything.
#14 lore raw

PATCH 1/1 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.

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.

#15 lore raw

PATCH 1/1 libxl: check that device model binary is executable

# HG changeset patch
# User Ian Campbell <[email protected]> # 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.

tools/libxl/libxl_dm.c +6 -1
diff -r 4d0e906543dc -r 1e3a986b6d29 tools/libxl/libxl_dm.c
@@ -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) {
#16 lore raw

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

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

Quoted 1 line

libxl: check that device model binary is executable.

Thanks, acked and applied all four.

Ian.

#17 lore raw

PATCH 1/1 libxl: remove redundant call to libxl_domain_device_model

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1304516240 -3600
# Node ID cba88f487f7c5a2d08b0dd5a484c0895c6dee17f # Parent 1e3a986b6d298628c6ef8839448860fd41771b2a
libxl: remove redundant call to libxl_domain_device_model

tools/libxl/libxl_dm.c +1 -3
diff -r 1e3a986b6d29 -r cba88f487f7c tools/libxl/libxl_dm.c
@@ -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;
#18 lore raw

PATCH 1/1 libxl: pass libxl__spawn_starting to libxl__spawn_spawn

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# 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.

tools/libxl/libxl_dm.c +2 -1
diff -r cba88f487f7c -r ec008dc43727 tools/libxl/libxl_dm.c
@@ -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 */
tools/libxl/libxl_exec.c +4 -4
diff -r cba88f487f7c -r ec008dc43727 tools/libxl/libxl_exec.c
@@ -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 */
 
tools/libxl/libxl_internal.h +4 -3
diff -r cba88f487f7c -r ec008dc43727 tools/libxl/libxl_internal.h
@@ -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:
#19 lore raw

PATCH 1/1 libxl: add statup checks to libxl__wait_for_device_model

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# 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.

tools/libxl/libxl.c +1 -1
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl.c
@@ -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);
tools/libxl/libxl_device.c +25 -5
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_device.c
@@ -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");
tools/libxl/libxl_dm.c +2 -4
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_dm.c
@@ -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;
 }
tools/libxl/libxl_dom.c +1 -1
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_dom.c
@@ -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)
     {
tools/libxl/libxl_exec.c +52 -8

103 lines not on this page · raw message

tools/libxl/libxl_internal.h +2 -0
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_internal.h
@@ -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,
tools/libxl/libxl_pci.c +8 -4
diff -r ec008dc43727 -r 36871b37a9bd tools/libxl/libxl_pci.c
@@ -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 
#20 lore raw

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

Quoted 12 lines · Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# 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).

Quoted 5 lines

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.

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

tools/libxl/libxl_exec.c +1 -0
@@ -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)
#21 lore raw

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

Ian Campbell writes ("Re: [Xen-devel] [PATCH 4 of 4] libxl: add statup checks to libxl__wait_for_device_model"):

Quoted 3 lines

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.

Quoted 10 lines
tools/libxl/libxl_exec.c +1 -0
@@ -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)
#22 lore raw

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

Quoted 7 lines · Ian Jackson

Ian Campbell writes ("Re: [Xen-devel] [PATCH 4 of 4] libxl: add statup checks to libxl__wait_for_device_model"):

Quoted 3 lines

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.

Quoted 13 lines

Ian.

Quoted 10 lines
tools/libxl/libxl_exec.c +1 -0
@@ -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)
#23 lore raw

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

In reply to Ian Campbell

Ian Campbell writes ("[Xen-devel] [PATCH 0 of 4] libxl: improve error handling when device model fails to start early on"):

Quoted 3 lines

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.

Quoted 5 lines

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.

#24 lore raw

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

Quoted 7 lines · Ian Jackson

Ian Campbell writes ("[Xen-devel] [PATCH 0 of 4] libxl: improve error handling when device model fails to start early on"):

Quoted 3 lines

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 <[email protected]>
        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.

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

Quoted 7 lines
Quoted 5 lines

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.

#25 lore raw
In reply to Bastian Blank
Quoted 4 lines · Bastian Blank
Quoted 3 lines · Ian Campbell
Quoted 1 line · Bastian Blank

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

Quoted 8 lines
Quoted 1 line

You mean close-on-exec?

Yes.

Quoted 1 line

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 <[email protected]>
# 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.

tools/libxl/libxl_bootloader.c +4 -0
diff -r 117680fafe0c -r 158e845f0d2c tools/libxl/libxl_bootloader.c
@@ -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;
tools/libxl/libxl_exec.c +57 -4

83 lines not on this page · raw message

tools/libxl/xl_cmdimpl.c +1 -1
diff -r 117680fafe0c -r 158e845f0d2c tools/libxl/xl_cmdimpl.c
@@ -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;
Signature · 3 lines
Ian Campbell

Is a person who blows up banks an econoclast?
#26 lore raw

Ian Campbell writes ("[PATCH] Re: [Xen-devel] Re: [Pkg-xen-devel] xen 4.1 blktap2 support"):

Quoted 5 lines

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.

#27 lore raw
Quoted 9 lines · Ian Jackson

Ian Campbell writes ("[PATCH] Re: [Xen-devel] Re: [Pkg-xen-devel] xen 4.1 blktap2 support"):

Quoted 5 lines

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:

Quoted 7 lines

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.

Quoted 3 lines

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.

Signature · 4 lines
Ian Campbell

The world really isn't any worse.  It's just that the news coverage
is so much better.
Acked-by
#28 lore raw

Ian Campbell writes ("Re: [PATCH] Re: [Xen-devel] Re: [Pkg-xen-devel] xen 4.1 blktap2 support"):

Quoted 2 lines

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.

Thanks,
Ian.

#30 lore raw
Quoted 4 lines · Bastian Blank
Quoted 1 line · Bastian Blank

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

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