Re: [PATCH v4 08/10] libxl: call hotplug scripts for disk devices from libxl

[Roger Pau Monne <roger.pau@citrix.com>]

Roger Pau Monne wrote:
> Since most of the needed work is already done in previous patches,
> this patch only contains the necessary code to call hotplug scripts
> for disk devices, that should be called when the device is added or
> removed from a guest.
>
> We will chain the launch of the disk hotplug scripts after the
> device_backend_callback callback, or directly from
> libxl__initiate_device_{add,remove} if the device is already in the
> desired state.
>
> Changes since v2:
>
>   * Added array size check with assert.
>
>   * Added NetBSD code (so compilation is not broken).
>
>   * Removed a check for null in device_hotplug_timeout_cb.
>
> Changes since v1:
>
>   * Moved all the event related code that was inside libxl_linux.c into
>     libxl_device.c, so the flow of the device addition/removal event is
>     all in the same file.
>
> Cc: Ian Jackson<ian.jackson@eu.citrix.com>
> Signed-off-by: Roger Pau Monne<roger.pau@citrix.com>
> ---
>   tools/hotplug/Linux/xen-backend.rules     |    6 +-
>   tools/hotplug/Linux/xen-hotplug-common.sh |    6 ++
>   tools/libxl/libxl.c                       |   10 +++
>   tools/libxl/libxl_device.c                |  121 ++++++++++++++++++++++++++++-
>   tools/libxl/libxl_internal.h              |   20 +++++
>   tools/libxl/libxl_linux.c                 |   98 +++++++++++++++++++++++
>   tools/libxl/libxl_netbsd.c                |    9 ++
>   7 files changed, 266 insertions(+), 4 deletions(-)
>
> diff --git a/tools/hotplug/Linux/xen-backend.rules b/tools/hotplug/Linux/xen-backend.rules
> index 405387f..d55ff11 100644
> --- a/tools/hotplug/Linux/xen-backend.rules
> +++ b/tools/hotplug/Linux/xen-backend.rules
> @@ -1,11 +1,11 @@
> -SUBSYSTEM=="xen-backend", KERNEL=="tap*", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
> -SUBSYSTEM=="xen-backend", KERNEL=="vbd*", RUN+="/etc/xen/scripts/block $env{ACTION}"
> +SUBSYSTEM=="xen-backend", KERNEL=="tap*", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/blktap $env{ACTION}"
> +SUBSYSTEM=="xen-backend", KERNEL=="vbd*", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/block $env{ACTION}"
>   SUBSYSTEM=="xen-backend", KERNEL=="vtpm*", RUN+="/etc/xen/scripts/vtpm $env{ACTION}"
>   SUBSYSTEM=="xen-backend", KERNEL=="vif2-*", RUN+="/etc/xen/scripts/vif2 $env{ACTION}"
>   SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ACTION=="online", RUN+="/etc/xen/scripts/vif-setup online type_if=vif"
>   SUBSYSTEM=="xen-backend", KERNEL=="vif-*", ACTION=="offline", RUN+="/etc/xen/scripts/vif-setup offline type_if=vif"
>   SUBSYSTEM=="xen-backend", KERNEL=="vscsi*", RUN+="/etc/xen/scripts/vscsi $env{ACTION}"
> -SUBSYSTEM=="xen-backend", ACTION=="remove", RUN+="/etc/xen/scripts/xen-hotplug-cleanup"
> +SUBSYSTEM=="xen-backend", ACTION=="remove", ENV{UDEV_CALL}="1", RUN+="/etc/xen/scripts/xen-hotplug-cleanup"
>   KERNEL=="evtchn", NAME="xen/%k"
>   SUBSYSTEM=="xen", KERNEL=="blktap[0-9]*", NAME="xen/%k", MODE="0600"
>   SUBSYSTEM=="blktap2", KERNEL=="blktap[0-9]*", NAME="xen/blktap-2/%k", MODE="0600"
> diff --git a/tools/hotplug/Linux/xen-hotplug-common.sh b/tools/hotplug/Linux/xen-hotplug-common.sh
> index 8f6557d..4a7bc73 100644
> --- a/tools/hotplug/Linux/xen-hotplug-common.sh
> +++ b/tools/hotplug/Linux/xen-hotplug-common.sh
> @@ -15,6 +15,12 @@
>   # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   #
>
> +# Hack to prevent the execution of hotplug scripts from udev if the domain
> +# has been launched from libxl
> +if [ -n "${UDEV_CALL}" ]&&  \
> +   xenstore-read "libxl/disable_udev">/dev/null 2>&1; then
> +    exit 0
> +fi
>
>   dir=$(dirname "$0")
>   . "$dir/hotplugpath.sh"
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2f27fd5..ccb5bdc 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1607,6 +1607,11 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
>               flexarray_append(back, "params");
>               flexarray_append(back, dev);
>
> +            flexarray_append(back, "script");
> +            flexarray_append(back, GCSPRINTF("%s/%s",
> +                                             libxl__xen_script_dir_path(),
> +                                             "block"));
> +
>               assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
>               break;
>           case LIBXL_DISK_BACKEND_TAP:
> @@ -1622,6 +1627,11 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
>                   libxl__device_disk_string_of_format(disk->format),
>                   disk->pdev_path));
>
> +            flexarray_append(back, "script");
> +            flexarray_append(back, GCSPRINTF("%s/%s",
> +                                             libxl__xen_script_dir_path(),
> +                                             "blktap"));
> +
>               /* now create a phy device to export the device to the guest */
>               goto do_backend_phy;
>           case LIBXL_DISK_BACKEND_QDISK:
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 9933cc2..8e1ec0f 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -569,6 +569,14 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
>   static void device_backend_cleanup(libxl__gc *gc,
>                                      libxl__ao_device *aodev);
>
> +static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev);
> +
> +static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
> +                                   pid_t pid, int status);
> +
> +static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
> +                                      const struct timeval *requested_abs);
> +
>   void libxl__initiate_device_add(libxl__egc *egc, libxl__ao_device *aodev)
>   {
>       STATE_AO_GC(aodev->ao);
> @@ -657,7 +665,7 @@ retry_transaction:
>
>    out_ok:
>       if (t) xs_transaction_end(ctx->xsh, t, 0);
> -    aodev->callback(egc, aodev);
> +    device_hotplug(egc, aodev);
>       return;
>   }
>
> @@ -689,6 +697,9 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
>           goto out;
>       }
>
> +    device_hotplug(egc, aodev);
> +    return;
> +
>   out:
>       aodev->rc = rc;
>       aodev->callback(egc, aodev);
> @@ -701,6 +712,114 @@ static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
>       libxl__ev_devstate_cancel(gc,&aodev->ds);
>   }
>
> +static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    char *be_path = libxl__device_backend_path(gc, aodev->dev);
> +    char **args, **env;

These should we initialised to NULL.

char **args = NULL, **env = NULL;

> +    int rc = 0;
> +    int hotplug;
> +
> +    /* Check if we have to execute hotplug scripts for this device
> +     * and return the necessary args/env vars for execution */
> +    hotplug = libxl__get_hotplug_script_info(gc, aodev->dev,&args,&env,
> +                                             aodev->action);
> +    switch (hotplug) {
> +    case 0:
> +        /* no hotplug script to execute */
> +        goto out;
> +    case 1:
> +        /* execute hotplug script */
> +        break;
> +    default:
> +        /* everything else is an error */
> +        LOG(ERROR, "unable to get args/env to execute hotplug script for "
> +                   "device %s", libxl__device_backend_path(gc, aodev->dev));
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    /* Set hotplug timeout */
> +    libxl__ev_time_init(&aodev->ev);
> +    rc = libxl__ev_time_register_rel(gc,&aodev->ev, device_hotplug_timeout_cb,
> +                                     LIBXL_HOTPLUG_TIMEOUT * 1000);
> +    if (rc) {
> +        LOG(ERROR, "unable to register timeout for hotplug device %s", be_path);
> +        goto out;
> +    }
> +
> +    aodev->what = GCSPRINTF("%s %s", args[0], args[1]);
> +    LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
> +    libxl__ev_child_init(&aodev->child);
> +
> +    /* fork and execute hotplug script */
> +    aodev->pid = libxl__ev_child_fork(gc,&aodev->child,
> +                                      device_hotplug_fork_cb);
> +    if (aodev->pid == -1) {
> +        LOG(ERROR, "unable to fork");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    if (!aodev->pid) {
> +        /* child */
> +        libxl__exec(gc, -1, -1, -1, args[0], args, env);
> +        /* notreached */
> +        abort();
> +    }
> +
> +    if (!libxl__ev_child_inuse(&aodev->child)) {
> +        /* hotplug launch failed */
> +        LOG(ERROR, "unable to launch hotplug script for device %s", be_path);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    return;
> +
> +out:
> +    libxl__ev_time_deregister(gc,&aodev->ev);
> +    aodev->rc = rc;
> +    aodev->callback(egc, aodev);
> +    return;
> +}
> +
> +static void device_hotplug_fork_cb(libxl__egc *egc, libxl__ev_child *child,
> +                                   pid_t pid, int status)
> +{
> +    libxl__ao_device *aodev = CONTAINER_OF(child, *aodev, child);
> +    STATE_AO_GC(aodev->ao);
> +
> +    libxl__ev_time_deregister(gc,&aodev->ev);
> +
> +    if (status) {
> +        libxl_report_child_exitstatus(CTX, aodev->rc ? LIBXL__LOG_ERROR
> +                                                     : LIBXL__LOG_WARNING,
> +                                      aodev->what, pid, status);
> +        aodev->rc = ERROR_FAIL;
> +    }
> +    aodev->callback(egc, aodev);
> +}
> +
> +static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
> +                                      const struct timeval *requested_abs)
> +{
> +    libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, ev);
> +    STATE_AO_GC(aodev->ao);
> +
> +    if (libxl__ev_child_inuse(&aodev->child)) {
> +        if (kill(aodev->pid, SIGKILL)) {
> +            LOGEV(ERROR, errno, "unable to kill hotplug script %s [%ld]",
> +                                aodev->what, (unsigned long)aodev->pid);
> +            goto out;
> +        }
> +    }
> +
> +out:
> +    libxl__ev_time_deregister(gc,&aodev->ev);
> +    return;
> +}
> +
>   static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aodev)
>   {
>       STATE_AO_GC(aodev->ao);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 45b776c..da5b02b 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -72,6 +72,7 @@
>
>   #define LIBXL_INIT_TIMEOUT 10
>   #define LIBXL_DESTROY_TIMEOUT 10
> +#define LIBXL_HOTPLUG_TIMEOUT 10
>   #define LIBXL_DEVICE_MODEL_START_TIMEOUT 10
>   #define LIBXL_XENCONSOLE_LIMIT 1048576
>   #define LIBXL_XENCONSOLE_PROTOCOL "vt100"
> @@ -1814,6 +1815,11 @@ struct libxl__ao_device {
>       int rc;
>       libxl__ev_devstate ds;
>       void *base;
> +    /* device hotplug execution */
> +    pid_t pid;
> +    char *what;
> +    libxl__ev_time ev;
> +    libxl__ev_child child;
>   };
>
>   /* Internal AO operation to connect a disk device */
> @@ -1842,6 +1848,20 @@ _hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aodev);
>   _hidden void libxl__initiate_device_remove(libxl__egc *egc,
>                                              libxl__ao_device *aodev);
>
> +/*
> + * libxl__get_hotplug_script_info returns the args and env that should
> + * be passed to the hotplug script for the requested device.
> + *
> + * Since a device might not need to execute any hotplug script, this function
> + * can return the following values:
> + *<  0: Error
> + * 0: No need to execute hotplug script
> + * 1: Execute hotplug script
> + */
> +_hidden int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
> +                                           char ***args, char ***env,
> +                                           libxl__device_action action);
> +
>   /*----- Domain destruction -----*/
>
>   /* Domain destruction has been split into two functions:
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 925248b..98cd25f 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -25,3 +25,101 @@ int libxl__try_phy_backend(mode_t st_mode)
>
>       return 1;
>   }
> +
> +/* Hotplug scripts helpers */
> +
> +static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
> +{
> +    char *be_path = libxl__device_backend_path(gc, dev);
> +    char *script;
> +    const char *type = libxl__device_kind_to_string(dev->backend_kind);
> +    char **env;
> +    int nr = 0, arraysize = 9;
> +
> +    script = libxl__xs_read(gc, XBT_NULL,
> +                            GCSPRINTF("%s/%s", be_path, "script"));
> +    if (!script) {
> +        LOGEV(ERROR, errno, "unable to read script from %s", be_path);
> +        return NULL;
> +    }
> +
> +    GCNEW_ARRAY(env, arraysize);
> +    env[nr++] = "script";
> +    env[nr++] = script;
> +    env[nr++] = "XENBUS_TYPE";
> +    env[nr++] = libxl__strdup(gc, type);
> +    env[nr++] = "XENBUS_PATH";
> +    env[nr++] = GCSPRINTF("backend/%s/%u/%d", type, dev->domid, dev->devid);
> +    env[nr++] = "XENBUS_BASE_PATH";
> +    env[nr++] = "backend";
> +    env[nr++] = NULL;
> +    assert(nr == arraysize);
> +
> +    return env;
> +}
> +
> +/* Hotplug scripts caller functions */
> +
> +static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
> +                               char ***args, char ***env,
> +                               libxl__device_action action)
> +{
> +    char *be_path = libxl__device_backend_path(gc, dev);
> +    char *script;
> +    int nr = 0, rc = 0, arraysize = 3;
> +
> +    script = libxl__xs_read(gc, XBT_NULL,
> +                            GCSPRINTF("%s/%s", be_path, "script"));
> +    if (!script) {
> +        LOGEV(ERROR, errno, "unable to read script from %s", be_path);
> +        rc = ERROR_FAIL;
> +        goto error;
> +    }
> +
> +    *env = get_hotplug_env(gc, dev);
> +    if (!*env) {
> +        rc = ERROR_FAIL;
> +        goto error;
> +    }
> +
> +    GCNEW_ARRAY(*args, arraysize);
> +    (*args)[nr++] = script;
> +    (*args)[nr++] = action == DEVICE_CONNECT ? "add" : "remove";
> +    (*args)[nr++] = NULL;
> +    assert(nr == arraysize);
> +
> +    rc = 0;
> +
> +error:
> +    return rc;
> +}
> +
> +int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
> +                                   char ***args, char ***env,
> +                                   libxl__device_action action)
> +{
> +    char *disable_udev = libxl__xs_read(gc, XBT_NULL, DISABLE_UDEV_PATH);
> +    int rc;
> +
> +    /* Check if we have to run hotplug scripts */
> +    if (!disable_udev) {
> +        rc = 0;
> +        goto out;
> +    }
> +
> +    switch (dev->backend_kind) {
> +    case LIBXL__DEVICE_KIND_VBD:
> +        rc = libxl__hotplug_disk(gc, dev, args, env, action);
> +        if (!rc) rc = 1;
> +        break;
> +    default:
> +        /* If no need to execute any hotplug scripts,
> +         * call the callback manually
> +         */
> +        rc = 0;
> +        break;
> +    }
> +
> +out:
> +    return rc;
> +}
> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 9e0ed6d..0f2cdaa 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -24,3 +24,12 @@ int libxl__try_phy_backend(mode_t st_mode)
>
>       return 0;
>   }
> +
> +/* Hotplug scripts caller functions */
> +
> +int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
> +                                   char ***args, char ***env,
> +                                   libxl__device_action action)
> +{
> +    return 0;
> +}
> \ No newline at end of file
> --
> 1.7.7.5 (Apple Git-26)
>