libxl: add support for hotplug script calling from libxl

# HG changeset patch
# User Roger Pau Monne <[email protected]> # Date 1317386335 -7200
# Node ID 9e8abd626484f82a95d0edc07834ae287bc9467a # Parent 23578c9942bcc8767adc4e435bb1fd1cd89f5e18
libxl: add support for image files for NetBSD

Created a helper function to detect if the OS is capable of using image files as phy backends. Create two OS specific files, and changed the Makefile to choose the correct one at compile time.

Signed-off-by Roger Pau Monne <[email protected]>

diff -r 23578c9942bc -r 9e8abd626484 tools/libxl/Makefile

tools/libxl/Makefile +2 -0

+endif
+
 LIBXL_LIBS += -lyajl
 
 LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \

diff -r 23578c9942bc -r 9e8abd626484 tools/libxl/libxl_device.c

tools/libxl/libxl_device.c +7 -8

@@ -138,15 +138,14 @@ static int disk_try_backend(disk_try_bac
               a->disk->format == LIBXL_DISK_FORMAT_EMPTY)) {
             goto bad_format;
         }
-        if (a->disk->format != LIBXL_DISK_FORMAT_EMPTY &&
-            !S_ISBLK(a->stab.st_mode)) {
-            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
-                       " unsuitable as phys path not a block device",
-                       a->disk->vdev);
-            return 0;
-        }
 
-        return backend;
+        if (libxl__try_phy_backend(a->stab.st_mode))
+            return backend;
+
+        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
+                   " unsuitable as phys path not a block device",
+                   a->disk->vdev);
+        return 0;
 
     case LIBXL_DISK_BACKEND_TAP:
         if (!libxl__blktap_enabled(a->gc)) {

diff -r 23578c9942bc -r 9e8abd626484 tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h +9 -0

@@ -252,6 +252,15 @@ _hidden int libxl__device_destroy(libxl_
 _hidden int libxl__devices_destroy(libxl__gc *gc, uint32_t domid, int force);
 _hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
 
+/*
+ * libxl__try_phy_backend - Check if there's support for the passed
+ * type of file using the PHY backend
+ * st_mode: mode_t of the file, as returned by stat function
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__try_phy_backend(mode_t st_mode);
+
 /* from libxl_pci */
 
 _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);

diff -r 23578c9942bc -r 9e8abd626484 tools/libxl/libxl_nophybackend.c

tools/libxl/libxl_nophybackend.c new file +27 -0

@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2011
+ * Author Roger Pau Monne <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+ 
+#include <sys/stat.h>
+
+#include "libxl_internal.h"
+ 
+int libxl__try_phy_backend(mode_t st_mode)
+{
+    if (!S_ISBLK(st_mode)) {
+        return 0;
+    }
+
+    return 1;
+}

diff -r 23578c9942bc -r 9e8abd626484 tools/libxl/libxl_phybackend.c

tools/libxl/libxl_phybackend.c new file +26 -0

@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2011
+ * Author Roger Pau Monne <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+ 
+#include <sys/stat.h>
+
+#include "libxl_internal.h"
+ 
+int libxl__try_phy_backend(mode_t st_mode)
+{
+    if (S_ISREG(st_mode) || S_ISBLK(st_mode))
+        return 1;
+
+    return 0;
+}

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

Quoted 20 lines

# HG changeset patch
# User Roger Pau Monne <[email protected]> # Date 1317386335 -7200
# Node ID 9e8abd626484f82a95d0edc07834ae287bc9467a # Parent  23578c9942bcc8767adc4e435bb1fd1cd89f5e18
libxl: add support for image files for NetBSD

Created a helper function to detect if the OS is capable of using image files as phy backends. Create two OS specific files, and changed the Makefile to choose the correct one at compile time.

Signed-off-by Roger Pau Monne <[email protected]>

diff -r 23578c9942bc -r 9e8abd626484 tools/libxl/Makefile

tools/libxl/Makefile    Fri Sep 30 14:38:55 2011 +0200 +0 -0

@@ -32,6 +32,12 @@ endif

 LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o
 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o

tools/libxl/Makefile    Fri Sep 30 14:38:55 2011 +0200

Quoted 4 lines

tools/libxl/Makefile    Fri Sep 30 14:38:55 2011 +0200 +4 -0

+ifeq ($(CONFIG_NetBSD),y)
+LIBXL_OBJS-y += libxl_phybackend.o
+else
+LIBXL_OBJS-y += libxl_nophybackend.o

phy vs nophy don't really make sense to me here, since in both cases the content relates to the phy backend.

Perhaps we need libxl_$(OS).c to contain os specific stuff?

2011/11/18 Ian Campbell <[email protected]>:

Quoted 29 lines · Roger Pau Monne

Quoted 17 lines

# HG changeset patch
# User Roger Pau Monne <[email protected]> # Date 1317386335 -7200
# Node ID 9e8abd626484f82a95d0edc07834ae287bc9467a # Parent 23578c9942bcc8767adc4e435bb1fd1cd89f5e18
libxl: add support for image files for NetBSD

Created a helper function to detect if the OS is capable of using image files as phy backends. Create two OS specific files, and changed the Makefile to choose the correct one at compile time.

Signed-off-by Roger Pau Monne <[email protected]>

diff -r 23578c9942bc -r 9e8abd626484 tools/libxl/Makefile

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 endif · line 38

Quoted 7 lines

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 +4 -0

@@ -32,6 +32,12 @@ endif
 LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o
 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o
 
+ifeq ($(CONFIG_NetBSD),y)
+LIBXL_OBJS-y += libxl_phybackend.o
+else
+LIBXL_OBJS-y += libxl_nophybackend.o

phy vs nophy don't really make sense to me here, since in both cases the content relates to the phy backend.

Quoted 1 line

Perhaps we need libxl_$(OS).c to contain os specific stuff?

A libxl_$(OS).c sounds interesting, I could put hotplug and backend OS specific code there, but I'm afraid it might get crowded and become difficult to understand. If I don't receive any other suggestions, I will create a libxl_netbsd.c and libxl_linux.c (and libxl_solaris.c?) and place the hotplug and backend helper functions there.

# HG changeset patch
# User Roger Pau Monne <[email protected]> # Date 1321609614 -3600
# Node ID ec94a7e4983ad6338db20fa0777a85507b582607 # Parent 9e8abd626484f82a95d0edc07834ae287bc9467a
libxl: add libxl__forkexec function to libxl_exec

Add a new function to libxl_exec that performs a fork and executes the passed arguments using libxl__exec.

Signed-off-by Roger Pau Monne <[email protected]>

diff -r 9e8abd626484 -r ec94a7e4983a tools/libxl/libxl_exec.c

tools/libxl/libxl_exec.c +34 -0

@@ -450,6 +450,40 @@ int libxl__spawn_check(libxl__gc *gc, li
     return ERROR_FAIL;
 }
 
+int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
+                    int stderrfd, char **args)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    int status;
+    int rc = 0;
+    pid_t pid = fork();
+
+    switch (pid) {
+    case -1:
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fork failed\n");
+        rc = -1;
+        break;
+    case 0:
+        libxl__exec(stdinfd, stdoutfd, stderrfd, args[0], args);
+        /* libxl__exec never returns */
+    default:
+        while (waitpid(pid, &status, 0) < 0) {
+            if (errno != EINTR) {
+                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "waitpid failed\n");
+                rc = -1;
+                break;
+            }
+        }
+        if (WIFEXITED(status)) {
+            rc = WEXITSTATUS(status);
+        } else {
+            rc = -1;
+        }
+        break;
+    }
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C

diff -r 9e8abd626484 -r ec94a7e4983a tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h +0 -0

@@ -400,6 +400,20 @@ _hidden int libxl__spawn_check(libxl__gc
 _hidden void libxl__exec(int stdinfd, int stdoutfd, int stderrfd,

tools/libxl/libxl_internal.h +6 -0

+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__forkexec(libxl__gc *gc, int stdinfd, int stdoutfd,
+                            int stderrfd, char **args);
+
 /* from xl_create */
 _hidden int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info, uint32_t *domid);
 _hidden int libxl__domain_build(libxl__gc *gc,

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

args[n] must be NULL, right?

Conveniently, I have already written the second function for you:
libxl_report_child_exitstatus :-).

2011/11/24 Ian Jackson <[email protected]>:

Quoted 2 lines

Conveniently, I have already written the second function for you:
libxl_report_child_exitstatus :-).

Why does the libxl_report_child_exitstatus function print "unexpectedly exited status zero", shouldn't it print something like "child exited successfully"? Or I'm not supposed to call it if WIFEXITED(status) && WEXITSTATUS(status) == 0?

# HG changeset patch
# User Roger Pau Monne <[email protected]> # Date 1321609740 -3600
# Node ID a77bba3717402fdf24a72d028ae0f3d482e6f427 # Parent ec94a7e4983ad6338db20fa0777a85507b582607
libxl: introduce libxl__wait_for_device_state

This is a generic function, that waits for xs watches to reach a certain state and then executes the passed helper function. Removed wait_for_dev_destroy and used this new function instead. This function will also be used by future patches that need to wait for the initialization of devices before executing hotplug scripts.

Signed-off-by Roger Pau Monne <[email protected]>

diff -r ec94a7e4983a -r a77bba371740 tools/libxl/libxl_device.c

tools/libxl/libxl_device.c +28 -12

@@ -370,7 +370,9 @@ int libxl__device_disk_dev_number(const 
  * Returns 0 if a device is removed, ERROR_* if an error
  * or timeout occurred.
  */
-static int wait_for_dev_destroy(libxl__gc *gc, struct timeval *tv)
+int libxl__wait_for_device_state(libxl__gc *gc, struct timeval *tv,
+                                 int state,
+                                 libxl__device_state_handler handler)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int nfds, rc;

@@ -395,17 +397,14 @@ start:
         default:
             l1 = xs_read_watch(ctx->xsh, &n);
             if (l1 != NULL) {
-                char *state = libxl__xs_read(gc, XBT_NULL,
+                char *sstate = libxl__xs_read(gc, XBT_NULL,
                                              l1[XS_WATCH_PATH]);
-                if (!state || atoi(state) == 6) {
-                    xs_unwatch(ctx->xsh, l1[0], l1[1]);
-                    xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
-                    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
-                               "Destroyed device backend at %s",
-                               l1[XS_WATCH_TOKEN]);
-                    rc = 0;
+                if (!sstate || atoi(sstate) == state) {
+                    /* Call handler function if present */
+                    if (handler)
+                        rc = handler(gc, l1, sstate);
                 } else {
-                    /* State is not "disconnected", continue waiting... */
+                    /* State is different than expected, continue waiting... */
                     goto start;
                 }
                 free(l1);

@@ -418,6 +417,23 @@ start:
 }
 
 /*
+ * Handler function for device destruction to be passed to
+ * libxl__wait_for_device_state
+ */
+static int destroy_device(libxl__gc *gc, char **l1, char *state)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+
+    xs_unwatch(ctx->xsh, l1[0], l1[1]);
+    xs_rm(ctx->xsh, XBT_NULL, l1[XS_WATCH_TOKEN]);
+    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
+               "Destroyed device backend at %s",
+               l1[XS_WATCH_TOKEN]);
+
+    return 0;
+}
+
+/*
  * Returns 0 (device already destroyed) or 1 (caller must
  * wait_for_dev_destroy) on success, ERROR_* on fail.
  */

@@ -458,7 +474,7 @@ retry_transaction:
         struct timeval tv;
         tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
         tv.tv_usec = 0;
-        rc = wait_for_dev_destroy(gc, &tv);
+        rc = libxl__wait_for_device_state(gc, &tv, 6, destroy_device);
         if (rc < 0) /* an error or timeout occurred, clear watches */
             xs_unwatch(ctx->xsh, state_path, be_path);
         xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(gc, dev));

@@ -566,7 +582,7 @@ int libxl__devices_destroy(libxl__gc *gc
         tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
         tv.tv_usec = 0;
         while (n_watches > 0) {
-            if (wait_for_dev_destroy(gc, &tv) < 0) {
+            if (libxl__wait_for_device_state(gc, &tv, 6, destroy_device) < 0) {
                 /* function returned ERROR_* */
                 break;
             } else {

diff -r ec94a7e4983a -r a77bba371740 tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h +26 -0

@@ -20,6 +20,7 @@
 #include <stdint.h>
 #include <stdarg.h>
 #include <stdlib.h>
+#include <sys/time.h>
 
 #include <xs.h>
 #include <xenctrl.h>

@@ -252,6 +253,31 @@ _hidden int libxl__device_destroy(libxl_
 _hidden int libxl__devices_destroy(libxl__gc *gc, uint32_t domid, int force);
 _hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state);
 
+/* Handler for the libxl__wait_for_device_state callback */
+/*
+ * libxl__device_state_handler - Handler for the libxl__wait_for_device_state
+ * gc: allocation pool
+ * l1: array containing the path and token
+ * state: string that contains the state of the device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+typedef int libxl__device_state_handler(libxl__gc *gc, char **l1, char *state);
+
+/*
+ * libxl__wait_for_device_state - waits a given time for a device to
+ * reach a given state
+ * gc: allocation pool
+ * tv: timeval struct containing the maximum time to wait
+ * state: state to wait for
+ * handler: callback function to execute when state is reached
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__wait_for_device_state(libxl__gc *gc, struct timeval *tv,
+                                         int state,
+                                         libxl__device_state_handler handler);
+
 /*
  * libxl__try_phy_backend - Check if there's support for the passed
  * type of file using the PHY backend

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

# HG changeset patch
# User Roger Pau Monne <[email protected]>
# Date 1321612154 -3600
# Node ID 4ad998fddb16a299cb230ea23ba944d84adbd2bf
# Parent a77bba3717402fdf24a72d028ae0f3d482e6f427
libxl: wait for devices to initialize upon addition to the domain.

Block waiting for devices to initialize (switch to state 2). This is necessary because we need to call the hotplug scripts when state is 2, otherwise the execution fails. Make use of the newly introduced function libxl__wait_for_device_state.

Signed-off-by Roger Pau Monne <[email protected]>

diff -r a77bba371740 -r 4ad998fddb16 tools/libxl/libxl.c

tools/libxl/libxl.c +39 -1

@@ -971,6 +971,8 @@ int libxl_device_disk_add(libxl_ctx *ctx
     flexarray_t *front;
     flexarray_t *back;
     char *dev;
+    char *be_path, *state_path, *state;
+    struct timeval tv;
     libxl__device device;
     int major, minor, rc;
 

@@ -1075,6 +1077,23 @@ int libxl_device_disk_add(libxl_ctx *ctx
                              libxl__xs_kvs_of_flexarray(&gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(&gc, front, front->count));
 
+    be_path = libxl__device_backend_path(&gc, &device);
+    state_path = libxl__sprintf(&gc, "%s/state", be_path);
+    state = libxl__xs_read(&gc, XBT_NULL, state_path);
+
+    if (atoi(state) != 2) {
+        xs_watch(ctx->xsh, state_path, be_path);
+        tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
+        tv.tv_usec = 0;
+        rc = libxl__wait_for_device_state(&gc, &tv, 2, NULL);
+        xs_unwatch(ctx->xsh, state_path, be_path);
+        if (rc < 0) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                       "unable to initialize disk device: %s\n",
+                       disk->pdev_path);
+            goto out_free;
+        }
+    }
     rc = 0;
 
 out_free:

@@ -1450,6 +1469,8 @@ int libxl_device_nic_add(libxl_ctx *ctx,
     flexarray_t *back;
     libxl__device device;
     char *dompath, **l;
+    char *be_path, *state_path, *state;
+    struct timeval tv;
     unsigned int nb, rc;
 
     front = flexarray_make(16, 1);

@@ -1518,8 +1539,25 @@ int libxl_device_nic_add(libxl_ctx *ctx,
                              libxl__xs_kvs_of_flexarray(&gc, back, back->count),
                              libxl__xs_kvs_of_flexarray(&gc, front, front->count));
 
-    /* FIXME: wait for plug */
+    be_path = libxl__device_backend_path(&gc, &device);
+    state_path = libxl__sprintf(&gc, "%s/state", be_path);
+    state = libxl__xs_read(&gc, XBT_NULL, state_path);
+
+    if (atoi(state) != 2) {
+        xs_watch(ctx->xsh, state_path, be_path);
+        tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
+        tv.tv_usec = 0;
+        rc = libxl__wait_for_device_state(&gc, &tv, 2, NULL);
+        xs_unwatch(ctx->xsh, state_path, be_path);
+        if (rc < 0) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                       "unable to initialize nic device: %s\n",
+                       nic->ifname);
+            goto out_free;
+        }
+    }
     rc = 0;
+
 out_free:
     flexarray_free(back);
     flexarray_free(front);

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

It would be better to use the XenbusStateFoo constants than the numbers. This also applies to the prviosu patch, I just didn't think of it...

Otherwise this looks good.

I'm a little concerned about the synchronous wait here (since it will serialise adding all devices). Eventually it should be possible to do all the adds and then wait for them all but this is dependent on the event infrastructure.

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c Fri Nov 18 11:29:14 2011 +0100 +18 -0

+{
+    int rc = 0;
+
+    switch(dev->kind) {
+    case LIBXL__DEVICE_KIND_VIF:
+        rc = libxl__nic_hotplug(gc, dev);
+        break;
+    case LIBXL__DEVICE_KIND_VBD:
+        rc = libxl__disk_hotplug(gc, dev);
+        break;
+    default:
+        break;
+    }
+
+    return rc;
+}
+
 int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
                              char **bents, char **fents)
 {

@@ -449,6 +467,7 @@ int libxl__device_remove(libxl__gc *gc,
     if (!state)
         goto out;
     if (atoi(state) != 4) {
+        libxl__device_execute_hotplug(gc, dev);
         libxl__device_destroy_tapdisk(gc, be_path);
         xs_rm(ctx->xsh, XBT_NULL, be_path);
         goto out;

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c Fri Nov 18 11:29:14 2011 +0100 +3 -0

+     */
+    libxl__device_execute_hotplug(gc, dev);
+
     xs_rm(ctx->xsh, XBT_NULL, be_path);
     xs_rm(ctx->xsh, XBT_NULL, fe_path);
 

diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_internal.h Fri Nov 18 11:29:14 2011 +0100 +28 -0

@@ -287,6 +287,34 @@ _hidden int libxl__wait_for_device_state
  */
 _hidden int libxl__try_phy_backend(mode_t st_mode);
 
+/* hotplug functions */
+/*
+ * libxl__device_execute_hotplug - generic function to execute hotplug scripts
+ * gc: allocation pool
+ * dev: reference to the device that executes the hotplug scripts
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device *dev);
+
+/*
+ * libxl__disk_hotplug - execute hotplug script for a disk type device
+ * gc: allocation pool
+ * dev: reference to the disk device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev);
+
+/*
+ * libxl__nic_hotplug - execute hotplug script for a nic type device
+ * gc: allocation pool
+ * dev: reference to the nic device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev);
+
 /* from libxl_pci */
 
 _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);

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

tools/libxl/Makefile      Fri Sep 30 14:38:55 2011 +0200 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpu

Quoted 19 lines

# HG changeset patch
# User Roger Pau Monne <[email protected]> # Date 1317386335 -7200
# Node ID 1d81d1c4c851c0b07696373304801df56a221e90 # Parent  4ad998fddb16a299cb230ea23ba944d84adbd2bf
libxl: execute hotplug scripts directly from libxl.

Added the necessary handlers to execute hotplug scripts when necessary from libxl. Split NetBSD and Linux hotplug calls into two separate files, because parameters for hotplug scripts are different. Linux has empty functions, since the calling of hotplug scripts is still done using udev.

Signed-off-by Roger Pau Monne <[email protected]>

diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/Makefile

tools/libxl/Makefile      Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/Makefile      Fri Nov 18 11:29:14 2011 +0100 +0 -0

@@ -34,8 +34,10 @@ LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpu

Should be:

Quoted 3 lines

tools/libxl/Makefile      Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/Makefile      Fri Nov 18 11:29:14 2011 +0100 +0 -0

  ifeq ($(CONFIG_NetBSD),y)

 LIBXL_OBJS-y += libxl_phybackend.o
+LIBXL_OBJS-y += hotplug_netbsd.o

  elsif ($(CONFIG_Linux),y)

Quoted 2 lines

 LIBXL_OBJS-y += libxl_nophybackend.o
+LIBXL_OBJS-y += hotplug_linux.o

  else
  $(error A message of some sort)

Quoted 1 line

 endif

tools/libxl/libxl_device.c        Fri Sep 30 14:38:55 2011 +0200 int libxl__parse_backend_path(libxl__gc · line 71

Quoted 8 lines

diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/libxl_device.c

tools/libxl/libxl_device.c        Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c        Fri Nov 18 11:29:14 2011 +0100 +1 -0

@@ -68,6 +68,24 @@ int libxl__parse_backend_path(libxl__gc
      return libxl__device_kind_from_string(strkind, &dev->backend_kind);
  }
 
+int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device *dev)

No need for a add/remove type parameter?

Quoted 32 lines

tools/libxl/libxl_device.c        Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c        Fri Nov 18 11:29:14 2011 +0100 +18 -0

+{
+    int rc = 0;
+
+    switch(dev->kind) {
+    case LIBXL__DEVICE_KIND_VIF:
+        rc = libxl__nic_hotplug(gc, dev);
+        break;
+    case LIBXL__DEVICE_KIND_VBD:
+        rc = libxl__disk_hotplug(gc, dev);
+        break;
+    default:
+        break;
+    }
+
+    return rc;
+}
+
  int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
                               char **bents, char **fents)
  {

@@ -449,6 +467,7 @@ int libxl__device_remove(libxl__gc *gc,
      if (!state)
          goto out;
      if (atoi(state) != 4) {
+        libxl__device_execute_hotplug(gc, dev);
          libxl__device_destroy_tapdisk(gc, be_path);
          xs_rm(ctx->xsh, XBT_NULL, be_path);
          goto out;

@@ -492,6 +511,12 @@ int libxl__device_destroy(libxl__gc *gc,
      char *be_path = libxl__device_backend_path(gc, dev);
      char *fe_path = libxl__device_frontend_path(gc, dev);
 

tools/libxl/libxl_device.c        Fri Sep 30 14:38:55 2011 +0200 int libxl__device_destroy(libxl__gc *gc, · line 516

Quoted 3 lines

tools/libxl/libxl_device.c        Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c        Fri Nov 18 11:29:14 2011 +0100 +3 -0

+    /*
+     * Run hotplug scripts, which will probably not be able to
+     * execute successfully since the device may still be plugged

What does this mean? If we don't expect it to work why are we calling them?

Quoted 43 lines

tools/libxl/libxl_device.c        Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c        Fri Nov 18 11:29:14 2011 +0100 +3 -0

+     */
+    libxl__device_execute_hotplug(gc, dev);
+
      xs_rm(ctx->xsh, XBT_NULL, be_path);
      xs_rm(ctx->xsh, XBT_NULL, fe_path);
 

diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h      Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_internal.h      Fri Nov 18 11:29:14 2011 +0100 +28 -0

@@ -287,6 +287,34 @@ _hidden int libxl__wait_for_device_state
   */
  _hidden int libxl__try_phy_backend(mode_t st_mode);
 
+/* hotplug functions */
+/*
+ * libxl__device_execute_hotplug - generic function to execute hotplug scripts
+ * gc: allocation pool
+ * dev: reference to the device that executes the hotplug scripts
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device *dev);
+
+/*
+ * libxl__disk_hotplug - execute hotplug script for a disk type device
+ * gc: allocation pool
+ * dev: reference to the disk device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev);
+
+/*
+ * libxl__nic_hotplug - execute hotplug script for a nic type device
+ * gc: allocation pool
+ * dev: reference to the nic device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev);
+
  /* from libxl_pci */
 

Quoted 6 lines

tools/libxl/libxl_internal.h      Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_internal.h      Fri Nov 18 11:29:14 2011 +0100 +0 -0

  _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);

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

2011/11/18 Ian Campbell <[email protected]>:

Quoted 23 lines · Roger Pau Monne

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpu

Quoted 19 lines

# HG changeset patch
# User Roger Pau Monne <[email protected]> # Date 1317386335 -7200
# Node ID 1d81d1c4c851c0b07696373304801df56a221e90 # Parent 4ad998fddb16a299cb230ea23ba944d84adbd2bf
libxl: execute hotplug scripts directly from libxl.

Added the necessary handlers to execute hotplug scripts when necessary from libxl. Split NetBSD and Linux hotplug calls into two separate files, because parameters for hotplug scripts are different. Linux has empty functions, since the calling of hotplug scripts is still done using udev.

Signed-off-by Roger Pau Monne <[email protected]>

diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/Makefile

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/Makefile Fri Nov 18 11:29:14 2011 +0100 +0 -0

@@ -34,8 +34,10 @@ LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpu

Should be:

Quoted 9 lines

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpu · line 36

Quoted 3 lines

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/Makefile Fri Nov 18 11:29:14 2011 +0100 +1 -0

 ifeq ($(CONFIG_NetBSD),y)
 LIBXL_OBJS-y += libxl_phybackend.o
+LIBXL_OBJS-y += hotplug_netbsd.o

elsif ($(CONFIG_Linux),y)

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpu · line 38

Quoted 2 lines

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/Makefile Fri Nov 18 11:29:14 2011 +0100 +1 -0

 LIBXL_OBJS-y += libxl_nophybackend.o
+LIBXL_OBJS-y += hotplug_linux.o

else
$(error A message of some sort)

Quoted 1 line

tools/libxl/Makefile Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/Makefile Fri Nov 18 11:29:14 2011 +0100 +0 -0

 endif

Done, I've moved PHY backend selection and hotplug specific functions to libxl_netbsd.c and libxl_linux.c, and added an error message in case someone tries to use a different OS.

Quoted 7 lines

Quoted 7 lines

diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/libxl_device.c

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c Fri Nov 18 11:29:14 2011 +0100 +0 -0

@@ -68,6 +68,24 @@ int libxl__parse_backend_path(libxl__gc
     return libxl__device_kind_from_string(strkind, &dev->backend_kind);
 }
 

Quoted 3 lines

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 int libxl__parse_backend_path(libxl__gc · line 71

Quoted 1 line

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c Fri Nov 18 11:29:14 2011 +0100 +1 -0

+int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device *dev)

No need for a add/remove type parameter?

Although you could pass this kind of parameter, the action to perform is based on the state of the device in the corresponding xenstore entry. State 2 means that the device should be connected, state 6 means the device should be disconnected.

Quoted 33 lines

Quoted 33 lines

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c Fri Nov 18 11:29:14 2011 +0100 +18 -0

+{
+    int rc = 0;
+
+    switch(dev->kind) {
+    case LIBXL__DEVICE_KIND_VIF:
+        rc = libxl__nic_hotplug(gc, dev);
+        break;
+    case LIBXL__DEVICE_KIND_VBD:
+        rc = libxl__disk_hotplug(gc, dev);
+        break;
+    default:
+        break;
+    }
+
+    return rc;
+}
+
 int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
                              char **bents, char **fents)
 {

@@ -449,6 +467,7 @@ int libxl__device_remove(libxl__gc *gc,
     if (!state)
         goto out;
     if (atoi(state) != 4) {
+        libxl__device_execute_hotplug(gc, dev);
         libxl__device_destroy_tapdisk(gc, be_path);
         xs_rm(ctx->xsh, XBT_NULL, be_path);
         goto out;

@@ -492,6 +511,12 @@ int libxl__device_destroy(libxl__gc *gc,
     char *be_path = libxl__device_backend_path(gc, dev);
     char *fe_path = libxl__device_frontend_path(gc, dev);
 

Quoted 6 lines

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 int libxl__device_destroy(libxl__gc *gc, · line 516

Quoted 3 lines

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c Fri Nov 18 11:29:14 2011 +0100 +3 -0

+    /*
+     * Run hotplug scripts, which will probably not be able to
+     * execute successfully since the device may still be plugged

What does this mean? If we don't expect it to work why are we calling them?

If you call the libxl_domain_destroy function with force == 1, the destroy process doesn't wait for devices to be disconnected, but we might as well try to execute hotplug scripts, since we don't know the actual state of the device. If we are lucky, the device might be disconnected (state == 6), and we should be able to execute hotplug scripts successfully.

Quoted 46 lines

Quoted 46 lines

tools/libxl/libxl_device.c Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_device.c Fri Nov 18 11:29:14 2011 +0100 +3 -0

+     */
+    libxl__device_execute_hotplug(gc, dev);
+
     xs_rm(ctx->xsh, XBT_NULL, be_path);
     xs_rm(ctx->xsh, XBT_NULL, fe_path);
 

diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h Fri Sep 30 14:38:55 2011 +0200 renamed from tools/libxl/libxl_internal.h Fri Nov 18 11:29:14 2011 +0100 +28 -0

@@ -287,6 +287,34 @@ _hidden int libxl__wait_for_device_state
  */
 _hidden int libxl__try_phy_backend(mode_t st_mode);
 
+/* hotplug functions */
+/*
+ * libxl__device_execute_hotplug - generic function to execute hotplug scripts
+ * gc: allocation pool
+ * dev: reference to the device that executes the hotplug scripts
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device *dev);
+
+/*
+ * libxl__disk_hotplug - execute hotplug script for a disk type device
+ * gc: allocation pool
+ * dev: reference to the disk device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev);
+
+/*
+ * libxl__nic_hotplug - execute hotplug script for a nic type device
+ * gc: allocation pool
+ * dev: reference to the nic device
+ *
+ * Returns 0 on success, and < 0 on error.
+ */
+_hidden int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev);
+
 /* from libxl_pci */
 
 _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int starting);

Quoted 7 lines

Quoted 4 lines

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

I was just looking at this again because someone was talking about hotplug issues on Linux and something new occurred to me.

Quoted 10 lines · Roger Pau Monne

[...]
+++ b/tools/libxl/hotplug_linux.c       Fri Sep 30 14:38:55 2011 +0200 [...]

tools/libxl/hotplug_linux.c new file +4 -0

+int libxl_disk_hotplug(libxl__gc *gc, libxl__device *dev)
[...]
+int libxl_nic_hotplug_connect(libxl__gc *gc, libxl__device *dev)
[...]
+++ b/tools/libxl/hotplug_netbsd.c      Fri Sep 30 14:38:55 2011 +0200
[...]
+int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev)

Quoted 2 lines

[...]
+int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev)

Although Linux doesn't do hotplug this way now I think it will/should in the future and the code will look very much like the netbsd one. So I think that the code which actually handles calling the script should probably be a generic function, only the selector on whether to do so needs to be platform specific.