Re: [Xen-devel] [PATCH v8 10/15] libxl: add option to choose who executes hotplug scripts

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-07-04 at 12:59 +0100, Roger Pau Monne wrote:
> Add and option to xl.conf file to decide if hotplug scripts are
> executed from the toolstack (xl) or from udev as it used to be in the
> past.
> 
> This option is only introduced in this patch, but it has no effect
> since the code to call hotplug scripts from libxl is introduced in a
> latter patch.
> 
> This choice will be saved in "libxl/disable_udev", as specified in the
> DISABLE_UDEV_PATH constant.
> 
> Changes since v2:
> 
>  * Change atoi(...) to !!atoi(...) to prevent returning negative
>    values from xenstore (which will be handled as errors).
> 
>  * Check for errors on the return value of libxl__hotplug_settings.
> 
> Changes since v1:
> 
>  * Used an auxiliary function to check for the current setting.
> 
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> ---
>  docs/man/xl.conf.pod.5       |    8 ++++++++
>  tools/examples/xl.conf       |    5 +++++
>  tools/libxl/libxl_create.c   |   38 +++++++++++++++++++++++++++++++++++++-
>  tools/libxl/libxl_dm.c       |    4 ++++
>  tools/libxl/libxl_internal.c |   19 +++++++++++++++++++
>  tools/libxl/libxl_internal.h |    3 +++
>  tools/libxl/libxl_types.idl  |    1 +
>  tools/libxl/xl.c             |    4 ++++
>  tools/libxl/xl.h             |    1 +
>  tools/libxl/xl_cmdimpl.c     |    1 +
>  10 files changed, 83 insertions(+), 1 deletions(-)
> 
> diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
> index 149430c..23932be 100644
> --- a/docs/man/xl.conf.pod.5
> +++ b/docs/man/xl.conf.pod.5
> @@ -55,6 +55,14 @@ default.
>  
>  Default: C<1>
>  
> +=item B<run_hotplug_scripts=BOOLEAN>
> +
> +If disabled hotplug scripts will be called from udev, as it used to
> +be in the previous releases. With the default option, hotplug scripts
> +will be launched by xl directly.
> +
> +Default: C<1>
> +
>  =item B<lockfile="PATH">
>  
>  Sets the path to the lock file used by xl to serialise certain
> diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
> index ebf057c..28ab796 100644
> --- a/tools/examples/xl.conf
> +++ b/tools/examples/xl.conf
> @@ -15,3 +15,8 @@
>  
>  # first block device to be used for temporary VM disk mounts
>  #blkdev_start="xvda"
> +
> +# default option to run hotplug scripts from xl
> +# if disabled the old behaviour will be used, and hotplug scripts will be
> +# launched by udev.
> +#run_hotplug_scripts=1
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 6f4c865..c584c4e 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -70,6 +70,8 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&c_info->oos, true);
>      }
>  
> +    libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true);
> +
>      return 0;
>  }
>  
> @@ -382,7 +384,7 @@ int libxl__domain_make(libxl__gc *gc, 
> libxl_domain_create_info *info,
>                         uint32_t *domid)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
> -    int flags, ret, rc;
> +    int flags, ret, rc, nb_vm;
>      char *uuid_string;
>      char *dom_path, *vm_path, *libxl_path;
>      struct xs_permissions roperm[2];
> @@ -503,6 +505,40 @@ retry_transaction:
>              libxl__sprintf(gc, "%s/hvmloader/generation-id-address", 
> dom_path),
>                          rwperm, ARRAY_SIZE(rwperm));
>  
> +    if (libxl_list_vm(ctx, &nb_vm) < 0) {

This will leak the returned list of vms...

> +        LOG(ERROR, "cannot get number of running guests");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    int hotplug_setting = libxl__hotplug_settings(gc, t);
> +    if (hotplug_setting < 0) {
> +        LOG(ERROR, "unable to get current hotplug scripts execution 
> setting");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    if (libxl_defbool_val(info->run_hotplug_scripts) != hotplug_setting &&
> +        (nb_vm - 1)) {
> +        LOG(ERROR, "cannot change hotplug execution option once set, "
> +                    "please shutdown all guests before changing it");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }

> +
> +    if (libxl_defbool_val(info->run_hotplug_scripts)) {
> +        rc = libxl__xs_write(gc, t, DISABLE_UDEV_PATH, "1");
> +        if (rc) {
> +            LOGE(ERROR, "unable to write %s = 1", DISABLE_UDEV_PATH);
> +            goto out;
> +        }
> +    } else {
> +        rc = xs_rm(ctx->xsh, t, DISABLE_UDEV_PATH);
> +        if (rc) {
> +            LOGE(ERROR, "unable to delete %s", DISABLE_UDEV_PATH);
> +            goto out;
> +        }
> +    }

You could use libxl__xs_{write,rm}_checked here I think.

> +
>      xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/uuid", vm_path), 
> uuid_string, strlen(uuid_string));
>      xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/name", vm_path), 
> info->name, strlen(info->name));
>  
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index b4a5f1f..612e9db 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -762,6 +762,10 @@ void libxl__spawn_stub_dm(libxl__egc *egc, 
> libxl__stub_dm_spawn_state *sdss)
>      dm_config->nics = guest_config->nics;
>      dm_config->num_nics = guest_config->num_nics;
>  
> +    libxl_defbool_set(
> +        &dm_config->c_info.run_hotplug_scripts,
> +        libxl_defbool_val(guest_config->c_info.run_hotplug_scripts));

This is the same as:

dm_config->c_info.run_hotplug_scripts = 
guest_config->c_info.run_hotplug_scripts;

Ian.


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