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

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

Ian Campbell wrote:
> 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@eu.citrix.com>
>> Acked-by: Ian Jackson<ian.jackson@eu.citrix.com>
>> Signed-off-by: Roger Pau Monne<roger.pau@citrix.com>
>> ---
>>   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...

Fixed.

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

Yes, when this was written libxl__xs_{write,rm}_checked wasn't there. 
Now I've changed it, although the rest of the code in this function 
still use xs_rm or libxl_xs_write.

>
>> +
>>       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;

Changed, thanks for the comments.