[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/5] libxl: call hotplug scripts from libxl for vbd



El 16/04/2012, a las 17:44, Ian Campbell escribió:
> On Mon, 2012-04-16 at 16:06 +0100, Roger Pau Monne wrote:
>> This is a rather big change, that adds the necessary machinery to perform
>> hotplug script calling from libxl for Linux. This patch launches the 
>> necessary
>> scripts to attach and detach PHY and TAP backend types (Qemu doesn't use 
>> hotplug
>> scripts).
>> 
>> Here's a list of the major changes introduced:
>> 
>> * libxl_device_disk_add makes use of the new event library and takes the
>>   optional parameter libxl_asyncop_how, to choose how the operation has to be
>>   issued (sync or async).
>> 
>> * libxl_device_disk_add waits for backend to switch to state 2 
>> (XenbusInitWait)
>>   and then launches the hotplug script.
>> 
>> * libxl__devices_destroy no longer calls libxl__device_destroy, and instead
>>   calls libxl__initiate_device_remove, so we can disconnect the device and
>>   execute the necessary hotplug scripts instead of just deleting the backend
>>   entries. So libxl__devices_destroy now uses the event library, and so does
>>   libxl_domain_destroy.
>> 
>> * Since libxl__devices_destroy calls multiple times
>>   libxl__initiate_device_remove, this function now returns a different value
>>   regarding the actions taken (if an event was added or not). The user has to
>>   call libxl__ao_complete after using this function if necessary.
>> 
>> * The internal API for hotplug scripts has been added, which consists of one
>>   function; libxl__device_hotplug that takes the device and the action to
>>   execute.
>> 
>> * Linux hotplug scripts are called by setting the necessary env variables to
>>   emulate udev rules, so there's no need to change them (although a rework to
>>   pass this as parameters insted of env variables would be suitable, so both
>>   NetBSD and Linux hotplug scripts take the same parameters).
>> 
>> * Added a check in xen-hotplug-common.sh, so scripts are only executed from
>>   udev when using xend. xl adds an execute_udev=n to xenstore device backend
>>   and passes XL_CALL=y to the env of the script call, and udev calls check 
>> that
>>   execute_udev is not set and XL_CALL is empty also.
>> 
>> I've also modified the python binding for pyxl_domain_destroy, that now take 
>> the
>> ao_how parameter, but I'm not really sure if it's done correctly, let's just 
>> say
>> that it compiles.
>> 
>> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
>> ---
>> tools/hotplug/Linux/xen-hotplug-common.sh |    6 ++
>> tools/libxl/Makefile                      |    3 +-
>> tools/libxl/libxl.c                       |  104 ++++++++++++++++++----
>> tools/libxl/libxl.h                       |    7 +-
>> tools/libxl/libxl_create.c                |    4 +-
>> tools/libxl/libxl_device.c                |  140 
>> +++++++++++++++++++++++++++--
>> tools/libxl/libxl_dm.c                    |    4 +-
>> tools/libxl/libxl_hotplug.c               |   67 ++++++++++++++
>> tools/libxl/libxl_internal.h              |   43 +++++++++-
>> tools/libxl/libxl_linux.c                 |  117 ++++++++++++++++++++++++
>> tools/libxl/xl_cmdimpl.c                  |   14 ++--
>> tools/python/xen/lowlevel/xl/xl.c         |    5 +-
>> 12 files changed, 474 insertions(+), 40 deletions(-)
>> create mode 100644 tools/libxl/libxl_hotplug.c
>> 
>> diff --git a/tools/hotplug/Linux/xen-hotplug-common.sh 
>> b/tools/hotplug/Linux/xen-hotplug-common.sh
>> index 8f6557d..dc3e867 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 form udev if the domain
> 
>                                                      from
>> +# has ben launched from libxl
> 
>        been


Done

> 
>> +execute_udev=`xenstore-read $XENBUS_PATH/execute_udev 2>/dev/null`
>> +if [ -n "$execute_udev" ] && [ -z "$XL_CALL" ]; then
>> +    exit 0
>> +fi
> 
> So, am I right that in some future world where we have got rid of xend
> and made this stuff work without udev everywhere (e.g. including driver
> domains) there is a path to deprecating this snippet without requiring
> everyone to go through some sort of transition?
> 
> Or are we stuck with this envvar forever? It's not a big deal but if we
> can invert it (e.g. by setting ENV{HOTPLUG_GATE}=${XENBUS}/evecute_udev
> in xen-backend.rules and ...if -n "${HOTPLUG_GATE}" && xenstore-read
> ${HOTPLUG_GATE}... etc) then that would be nicer?


I will change it to something like:

if [ -z "${HOTPLUG_GATE}" ] && `xenstore-read "$HOTPLUG_GATE" 2>/dev/null` && \
   [ -z "$XL_CALL" ]; then

and then set HOTPLUG_GATE from udev rules. I will change the xenstore gate to 
"disable_udev", which makes more sense, since we will be setting 
"disable_udev=y" or nothing.

>> dir=$(dirname "$0")
>> . "$dir/hotplugpath.sh"
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 16ebef3..fb4fbf2 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -1062,13 +1062,15 @@ void libxl_evdisable_disk_eject(libxl_ctx *ctx, 
>> libxl_evgen_disk_eject *evg) {
>>     GC_FREE;
>> }
>> 
>> -int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid)
>> +int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid,
>> +                         const libxl_asyncop_how *ao_how)
>> {
>> -    GC_INIT(ctx);
>> +    AO_CREATE(ctx, domid, ao_how);
>>     char *dom_path;
>>     char *vm_path;
>>     char *pid;
>>     int rc, dm_present;
>> +    int rc_ao = 0;
>> 
>>     rc = libxl_domain_info(ctx, NULL, domid);
>>     switch(rc) {
>> @@ -1110,7 +1112,8 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t 
>> domid)
>> 
>>         libxl__qmp_cleanup(gc, domid);
>>     }
>> -    if (libxl__devices_destroy(gc, domid) < 0)
>> +    rc_ao = libxl__devices_destroy(egc, ao, domid);
>> +    if (rc_ao < 0)
>>         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>>                    "libxl__devices_destroy failed for %d", domid);
>> 
>> @@ -1133,9 +1136,10 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t 
>> domid)
>>         goto out;
>>     }
>>     rc = 0;
>> +
>> out:
>> -    GC_FREE;
>> -    return rc;
>> +    if (rc_ao) return AO_ABORT(rc_ao);
>> +    return AO_INPROGRESS;
>> }
>> 
>> int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, 
>> libxl_console_type type)
>> @@ -1313,9 +1317,11 @@ static int libxl__device_from_disk(libxl__gc *gc, 
>> uint32_t domid,
>>     return 0;
>> }
>> 
>> -int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
>> *disk)
>> +int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
>> +                          libxl_device_disk *disk,
>> +                          const libxl_asyncop_how *ao_how)
>> {
>> -    GC_INIT(ctx);
>> +    AO_CREATE(ctx, domid, ao_how);
>>     flexarray_t *front;
>>     flexarray_t *back;
>>     char *dev;
>> @@ -1330,7 +1336,7 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t 
>> domid, libxl_device_disk *dis
>>         rc = ERROR_NOMEM;
>>         goto out;
>>     }
>> -    back = flexarray_make(16, 1);
>> +    back = flexarray_make(20, 1);
>>     if (!back) {
>>         rc = ERROR_NOMEM;
>>         goto out_free;
>> @@ -1361,6 +1367,11 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t 
>> domid, libxl_device_disk *dis
>>             flexarray_append(back, "params");
>>             flexarray_append(back, dev);
>> 
>> +            flexarray_append(back, "script");
>> +            flexarray_append(back, libxl__sprintf(gc, "%s/%s",
>> +                                                  
>> libxl_xen_script_dir_path(),
>> +                                                  "block"));
>> +
>>             assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
>>             break;
>>         case LIBXL_DISK_BACKEND_TAP:
>> @@ -1374,6 +1385,11 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t 
>> domid, libxl_device_disk *dis
>>                 libxl__device_disk_string_of_format(disk->format),
>>                 disk->pdev_path));
>> 
>> +            flexarray_append(back, "script");
>> +            flexarray_append(back, libxl__sprintf(gc, "%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:
>> @@ -1406,6 +1422,9 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t 
>> domid, libxl_device_disk *dis
>>     flexarray_append(back, disk->readwrite ? "w" : "r");
>>     flexarray_append(back, "device-type");
>>     flexarray_append(back, disk->is_cdrom ? "cdrom" : "disk");
>> +    /* compatibility addon to keep udev rules */
>> +    flexarray_append(back, "execute_udev");
>> +    flexarray_append(back, "n");
>> 
>>     flexarray_append(front, "backend-id");
>>     flexarray_append(front, libxl__sprintf(gc, "%d", disk->backend_domid));
>> @@ -1420,14 +1439,23 @@ int libxl_device_disk_add(libxl_ctx *ctx, uint32_t 
>> domid, libxl_device_disk *dis
>>                              libxl__xs_kvs_of_flexarray(gc, back, 
>> back->count),
>>                              libxl__xs_kvs_of_flexarray(gc, front, 
>> front->count));
>> 
>> +    if (disk->backend == LIBXL_DISK_BACKEND_PHY ||
>> +        disk->backend == LIBXL_DISK_BACKEND_TAP) {
>> +        rc = libxl__initiate_device_add(egc, ao, &device);
>> +        if (rc) goto out_free;
>> +    } else {
>> +        /* no need to execute hotplug scripts */
>> +        libxl__ao_complete(egc, ao, 0);
>> +    }
> 
> This would be better as a switch, since it would make it more obvious
> that the "else" is actually "case LIBXL_DISK_BACKEND_QDISK" etc.
> 

Yes

> [...]
>> +
>> +/*
>> + * Return codes:
>> + * 1 Success and event(s) HAVE BEEN added
>> + * 0 Success and NO events where added
> 
>                              were
> 
> (I saw a few of these)

Will do a grep to see what I can find…

Thanks for the review.

> 
>> + * < 0 Error
>> + *
>> + * Since this function can be called several times, and several events
>> + * can be added to the same context, the user has to call
>> + * libxl__ao_complete when necessary (if no events are added).
>> + */
>> int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
>>                                   libxl__device *dev)
>> {
>> @@ -392,7 +461,6 @@ int libxl__initiate_device_remove(libxl__egc *egc, 
>> libxl__ao *ao,
> [...]
> 
> 



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.