Re: [Xen-devel] [PATCH V2] libxl: make libxl_cdrom_insert async

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

I forgot to say... this is actually based on Roger's series because I
was expecting to need async disk add/remove, but as it turns out I don't
so I could rebase before that series.

On Wed, 2012-07-25 at 10:12 +0100, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@xxxxxxxxxx>
> # Date 1343207529 -3600
> # Node ID 4cb46317b245e0d4c729e3500bf03fdecf4ae4fa
> # Parent  fe5c863907a4988cd3f98087c393e23125d3ee15
> libxl: make libxl_cdrom_insert async.
> 
> This functionality is a bit of a mess and several configurations are
> not properly supported.
> 
> The protocol for changing is basically to change the params node in
> the disk xenstore backend. There is no interlock or error reporting in
> this protocol. Completely removing the device and recreating it is not
> necessary nor expected. For reference the equivalent xend code is
> tools/python/xen/xend/server/blkif.py::BlkifController::reconfigureDevice().
> 
> Device model stub domains are not supported. There appears to be no
> way correctly to do a media change on the emulated device while also
> changing the stub domains PV backend to point to the new
> backend. Reworking this is a significant task deferred until 4.3. xend
> (via the equivalent "xm block-configure" functionality) also does not
> support media change for stub domains (confirmed by code inspection
> and experiment). Unlike xend this version errors out instead of
> silently not achieving anything in this case.
> 
> There is no support for qemu-xen (upstream) media change. I expect
> this is supported on the qemu side and required QMP plumbing on the
> libxl side. Again this is deferred until 4.3.
> 
> On the plus side the current implementation is trivially "asynchronous".
> 
> Adds a libxl__xs_writev_atonce helper to write a key-value list to
> xenstore in one go.
> 
> Tested with Windows 7.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
> 
> v2:
>  - use GCSPRINTF and libxl__strup(NOGC,...)
>  - remove incorrect comment copied from switch_logdirty_xswatch
>  - don't clear then set the prarms key -- just set it
> 
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/libxl.c     Wed Jul 25 10:12:09 2012 +0100
> @@ -2131,17 +2131,49 @@ int libxl_device_disk_getinfo(libxl_ctx 
>      return 0;
>  }
>  
> -int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
> *disk)
> +int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
> *disk,
> +                       const libxl_asyncop_how *ao_how)
>  {
> -    int num, i;
> -    uint32_t stubdomid;
> -    libxl_device_disk *disks;
> -    int ret = ERROR_FAIL;
> -
> -    if (!disk->pdev_path) {
> -        disk->pdev_path = strdup("");
> -        disk->format = LIBXL_DISK_FORMAT_EMPTY;
> +    AO_CREATE(ctx, domid, ao_how);
> +    int num = 0, i;
> +    libxl_device_disk *disks = NULL;
> +    int rc, dm_ver;
> +
> +    libxl__device device;
> +    const char * path;
> +
> +    flexarray_t *insert = NULL;
> +
> +    libxl_domain_type type = libxl__domain_type(gc, domid);
> +    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> +        rc = ERROR_FAIL;
> +        goto out;
>      }
> +    if (type != LIBXL_DOMAIN_TYPE_HVM) {
> +        LOG(ERROR, "cdrom-insert requires an HVM domain");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    if (libxl_get_stubdom_id(ctx, domid) != 0) {
> +        LOG(ERROR, "cdrom-insert doesn't work for stub domains");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    dm_ver = libxl__device_model_version_running(gc, domid);
> +    if (dm_ver == -1) {
> +        LOG(ERROR, "cannot determine device model version");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    if (dm_ver != LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
> +        LOG(ERROR, "cdrom-insert does not work with %s",
> +            libxl_device_model_version_to_string(dm_ver));
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
>      disks = libxl_device_disk_list(ctx, domid, &num);
>      for (i = 0; i < num; i++) {
>          if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev))
> @@ -2150,25 +2182,52 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
>      }
>      if (i == num) {
>          LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not found");
> +        rc = ERROR_FAIL;
>          goto out;
>      }
>  
> -    ret = 0;
> -
> -    libxl_device_disk_remove(ctx, domid, disks + i, 0);
> -    /* fixme-ao */
> -    libxl_device_disk_add(ctx, domid, disk, 0);
> -    stubdomid = libxl_get_stubdom_id(ctx, domid);
> -    if (stubdomid) {
> -        libxl_device_disk_remove(ctx, stubdomid, disks + i, 0);
> -        /* fixme-ao */
> -        libxl_device_disk_add(ctx, stubdomid, disk, 0);
> +    rc = libxl__device_disk_setdefault(gc, disk);
> +    if (rc) goto out;
> +
> +    if (!disk->pdev_path) {
> +        disk->pdev_path = libxl__strdup(NOGC, "");
> +        disk->format = LIBXL_DISK_FORMAT_EMPTY;
>      }
> +
> +    rc = libxl__device_from_disk(gc, domid, disk, &device);
> +    if (rc) goto out;
> +    path = libxl__device_backend_path(gc, &device);
> +
> +    insert = flexarray_make(4, 1);
> +
> +    flexarray_append_pair(insert, "type",
> +                          
> libxl__device_disk_string_of_backend(disk->backend));
> +    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
> +        flexarray_append_pair(insert, "params",
> +                        GCSPRINTF("%s:%s",
> +                            
> libxl__device_disk_string_of_format(disk->format),
> +                            disk->pdev_path));
> +    else
> +        flexarray_append_pair(insert, "params", "");
> +
> +    rc = libxl__xs_writev_atonce(gc, path,
> +                        libxl__xs_kvs_of_flexarray(gc, insert, 
> insert->count));
> +    if (rc) goto out;
> +
> +    /* success, no actual async */
> +    libxl__ao_complete(egc, ao, 0);
> +
> +    rc = 0;
> +
>  out:
>      for (i = 0; i < num; i++)
>          libxl_device_disk_dispose(&disks[i]);
>      free(disks);
> -    return ret;
> +
> +    if (insert) flexarray_free(insert);
> +
> +    if (rc) return AO_ABORT(rc);
> +    return AO_INPROGRESS;
>  }
>  
>  /* libxl__alloc_vdev only works on the local domain, that is the domain
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h     Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/libxl.h     Wed Jul 25 10:12:09 2012 +0100
> @@ -693,7 +693,8 @@ int libxl_device_disk_getinfo(libxl_ctx 
>   * Insert a CD-ROM device. A device corresponding to disk must already
>   * be attached to the guest.
>   */
> -int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
> *disk);
> +int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
> *disk,
> +                       const libxl_asyncop_how *ao_how);
>  
>  /* Network Interfaces */
>  int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic 
> *nic,
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl_internal.h
> --- a/tools/libxl/libxl_internal.h    Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/libxl_internal.h    Wed Jul 25 10:12:09 2012 +0100
> @@ -501,8 +501,13 @@ _hidden int libxl__remove_file_or_direct
>  
>  _hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array, 
> int length);
>  
> +/* treats kvs as pairs of keys and values and writes each to dir. */
>  _hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
>                               const char *dir, char **kvs);
> +/* _atonce creates a transaction and writes all keys at once */
> +_hidden int libxl__xs_writev_atonce(libxl__gc *gc,
> +                             const char *dir, char **kvs);
> +
>  _hidden int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
>                 const char *path, const char *fmt, ...) PRINTF_ATTRIBUTE(4, 
> 5);
>     /* Each fn returns 0 on success.
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/libxl_xshelp.c
> --- a/tools/libxl/libxl_xshelp.c      Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/libxl_xshelp.c      Wed Jul 25 10:12:09 2012 +0100
> @@ -61,6 +61,31 @@ int libxl__xs_writev(libxl__gc *gc, xs_t
>      return 0;
>  }
>  
> +int libxl__xs_writev_atonce(libxl__gc *gc,
> +                            const char *dir, char *kvs[])
> +{
> +    int rc;
> +    xs_transaction_t t = XBT_NULL;
> +
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) goto out;
> +
> +        rc = libxl__xs_writev(gc, t, dir, kvs);
> +        if (rc) goto out;
> +
> +        rc = libxl__xs_transaction_commit(gc, &t);
> +        if (!rc) break;
> +        if (rc<0) goto out;
> +    }
> +
> +out:
> +    libxl__xs_transaction_abort(gc, &t);
> +
> +    return rc;
> +
> +}
> +
>  int libxl__xs_write(libxl__gc *gc, xs_transaction_t t,
>                      const char *path, const char *fmt, ...)
>  {
> diff -r fe5c863907a4 -r 4cb46317b245 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c        Wed Jul 25 09:48:09 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c        Wed Jul 25 10:12:09 2012 +0100
> @@ -2000,7 +2000,7 @@ start:
>  
>          case LIBXL_EVENT_TYPE_DISK_EJECT:
>              /* XXX what is this for? */
> -            libxl_cdrom_insert(ctx, domid, &event->u.disk_eject.disk);
> +            libxl_cdrom_insert(ctx, domid, &event->u.disk_eject.disk, NULL);
>              break;
>  
>          default:;
> @@ -2220,7 +2220,7 @@ static void cd_insert(const char *dom, c
>  
>      disk.backend_domid = 0;
>  
> -    libxl_cdrom_insert(ctx, domid, &disk);
> +    libxl_cdrom_insert(ctx, domid, &disk, NULL);
>  
>      libxl_device_disk_dispose(&disk);
>      free(buf);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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