[PATCH] libxl: make libxl_cdrom_insert async

[PATCH] libxl: make libxl_cdrom_insert async

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1343146187 -3600
# Node ID a242083e323a434bfc950a70bb0027185f1d0952
# Parent  9ff37d4565de26bbc6b0771405d413020cbf4d47
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@citrix.com>

diff -r 9ff37d4565de -r a242083e323a tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Jul 24 14:08:09 2012 +0100
+++ b/tools/libxl/libxl.c	Tue Jul 24 17:09:47 2012 +0100
@@ -2131,17 +2131,50 @@ 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 *eject = NULL;
+    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 +2183,60 @@ 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 = strdup("");
+        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);
+
+    eject = flexarray_make(4, 1);
+
+    flexarray_append_pair(eject, "type",
+                          libxl__device_disk_string_of_backend(disk->backend));
+    flexarray_append_pair(eject, "params", "");
+    rc = libxl__xs_writev_atonce(gc, path,
+                        libxl__xs_kvs_of_flexarray(gc, eject, eject->count));
+    if (rc) goto out;
+
+    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
+    {
+        insert = flexarray_make(4, 1);
+
+        flexarray_append_pair(insert, "type",
+                        libxl__device_disk_string_of_backend(disk->backend));
+        flexarray_append_pair(insert, "params",
+                        libxl__sprintf(gc, "%s:%s",
+                            libxl__device_disk_string_of_format(disk->format),
+                            disk->pdev_path));
+        rc = libxl__xs_writev_atonce(gc, path,
+                        libxl__xs_kvs_of_flexarray(gc, insert, insert->count));
+    }
+
+    /* 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 (eject) flexarray_free(eject);
+    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 9ff37d4565de -r a242083e323a tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Tue Jul 24 14:08:09 2012 +0100
+++ b/tools/libxl/libxl.h	Tue Jul 24 17:09:47 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 9ff37d4565de -r a242083e323a tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Tue Jul 24 14:08:09 2012 +0100
+++ b/tools/libxl/libxl_internal.h	Tue Jul 24 17:09:47 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 9ff37d4565de -r a242083e323a tools/libxl/libxl_xshelp.c
--- a/tools/libxl/libxl_xshelp.c	Tue Jul 24 14:08:09 2012 +0100
+++ b/tools/libxl/libxl_xshelp.c	Tue Jul 24 17:09:47 2012 +0100
@@ -61,6 +61,35 @@ 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:
+
+    /* rc < 0: error
+     * rc == 0: ok, we are done
+     * rc == +1: need to keep waiting
+     */
+    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 9ff37d4565de -r a242083e323a tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Tue Jul 24 14:08:09 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Tue Jul 24 17:09:47 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);

Re: [PATCH] libxl: make libxl_cdrom_insert async

[Ian Jackson]

Ian Campbell writes ("[PATCH] libxl: make libxl_cdrom_insert async"):
> libxl: make libxl_cdrom_insert async.
> 
> This functionality is a bit of a mess and several configurations are
> not properly supported.
>
> [explanation]

Urgh.

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> diff -r 9ff37d4565de -r a242083e323a tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Tue Jul 24 14:08:09 2012 +0100
> +++ b/tools/libxl/libxl.c	Tue Jul 24 17:09:47 2012 +0100
> @@ -2131,17 +2131,50 @@ 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)
>  {
...
> +    if (!disk->pdev_path) {
> +        disk->pdev_path = strdup("");

Don't you mean libxl__strdup ?  And presumably you should free the
previous pdev_path ?

(TBH this massaging of the caller's libxl_device_disk is a bit
unfriendly but I don't think getting rid of that is critical at this
stage.  We can constify it later, in 4.3.)

> +        disk->format = LIBXL_DISK_FORMAT_EMPTY;
>      }
...
> +    flexarray_append_pair(eject, "type",
> +                          libxl__device_disk_string_of_backend(disk->backend));
> +    flexarray_append_pair(eject, "params", "");
> +    rc = libxl__xs_writev_atonce(gc, path,
> +                        libxl__xs_kvs_of_flexarray(gc, eject, eject->count));
> +    if (rc) goto out;
> +
> +    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
> +    {
> +        insert = flexarray_make(4, 1);
> +
> +        flexarray_append_pair(insert, "type",
> +                        libxl__device_disk_string_of_backend(disk->backend));
> +        flexarray_append_pair(insert, "params",
> +                        libxl__sprintf(gc, "%s:%s",

GCSPRINTF ?

> +                            libxl__device_disk_string_of_format(disk->format),
> +                            disk->pdev_path));
> +        rc = libxl__xs_writev_atonce(gc, path,
> +                        libxl__xs_kvs_of_flexarray(gc, insert, insert->count));
> +    }

This doesn't look actually /incorrect/ but I'm a bit suspicious: if
changing to a new non-empty disk, you first write params := "", in one
xenstore transaction, and then the non-empty value in a second
transaction.  Why ?

> diff -r 9ff37d4565de -r a242083e323a tools/libxl/libxl_xshelp.c
> --- a/tools/libxl/libxl_xshelp.c	Tue Jul 24 14:08:09 2012 +0100
> +++ b/tools/libxl/libxl_xshelp.c	Tue Jul 24 17:09:47 2012 +0100
> @@ -61,6 +61,35 @@ int libxl__xs_writev(libxl__gc *gc, xs_t
>      return 0;
>  }
>  
> +int libxl__xs_writev_atonce(libxl__gc *gc,
> +                            const char *dir, char *kvs[])
> +{
...
> +out:
> +
> +    /* rc < 0: error
> +     * rc == 0: ok, we are done
> +     * rc == +1: need to keep waiting
> +     */

This comment has crept across from switch_logdirty_xswatch (which is a
watch callback function) and isn't correct here.


Thanks,
Ian.

Re: [PATCH] libxl: make libxl_cdrom_insert async

[Ian Campbell]

On Tue, 2012-07-24 at 17:49 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH] libxl: make libxl_cdrom_insert async"):
> > libxl: make libxl_cdrom_insert async.
> > 
> > This functionality is a bit of a mess and several configurations are
> > not properly supported.
> >
> > [explanation]
> 
> Urgh.
> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > diff -r 9ff37d4565de -r a242083e323a tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Tue Jul 24 14:08:09 2012 +0100
> > +++ b/tools/libxl/libxl.c	Tue Jul 24 17:09:47 2012 +0100
> > @@ -2131,17 +2131,50 @@ 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)
> >  {
> ...
> > +    if (!disk->pdev_path) {
> > +        disk->pdev_path = strdup("");
> 
> Don't you mean libxl__strdup ?  And presumably you should free the
> previous pdev_path ?
> 
> (TBH this massaging of the caller's libxl_device_disk is a bit
> unfriendly but I don't think getting rid of that is critical at this
> stage.  We can constify it later, in 4.3.)
> 
> > +        disk->format = LIBXL_DISK_FORMAT_EMPTY;
> >      }
> ...
> > +    flexarray_append_pair(eject, "type",
> > +                          libxl__device_disk_string_of_backend(disk->backend));
> > +    flexarray_append_pair(eject, "params", "");
> > +    rc = libxl__xs_writev_atonce(gc, path,
> > +                        libxl__xs_kvs_of_flexarray(gc, eject, eject->count));
> > +    if (rc) goto out;
> > +
> > +    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
> > +    {
> > +        insert = flexarray_make(4, 1);
> > +
> > +        flexarray_append_pair(insert, "type",
> > +                        libxl__device_disk_string_of_backend(disk->backend));
> > +        flexarray_append_pair(insert, "params",
> > +                        libxl__sprintf(gc, "%s:%s",
> 
> GCSPRINTF ?

Good idea.

> > +                            libxl__device_disk_string_of_format(disk->format),
> > +                            disk->pdev_path));
> > +        rc = libxl__xs_writev_atonce(gc, path,
> > +                        libxl__xs_kvs_of_flexarray(gc, insert, insert->count));
> > +    }
> 
> This doesn't look actually /incorrect/ but I'm a bit suspicious: if
> changing to a new non-empty disk, you first write params := "", in one
> xenstore transaction, and then the non-empty value in a second
> transaction.  Why ?

Mostly because xend did it that way, I agree that it seems pointless
without adding any additional interlock to the protocol.

At one point I was hoping that copying xend closely would lead to
working stubdom support etc, but that turned out to be a pipe dream.

Just writing the appropriate key once did seem to work, at least in the
cases where it works at all, in my experiments so I guess I might as
well change to that here.

> > diff -r 9ff37d4565de -r a242083e323a tools/libxl/libxl_xshelp.c
> > --- a/tools/libxl/libxl_xshelp.c	Tue Jul 24 14:08:09 2012 +0100
> > +++ b/tools/libxl/libxl_xshelp.c	Tue Jul 24 17:09:47 2012 +0100
> > @@ -61,6 +61,35 @@ int libxl__xs_writev(libxl__gc *gc, xs_t
> >      return 0;
> >  }
> >  
> > +int libxl__xs_writev_atonce(libxl__gc *gc,
> > +                            const char *dir, char *kvs[])
> > +{
> ...
> > +out:
> > +
> > +    /* rc < 0: error
> > +     * rc == 0: ok, we are done
> > +     * rc == +1: need to keep waiting
> > +     */
> 
> This comment has crept across from switch_logdirty_xswatch (which is a
> watch callback function) and isn't correct here.

Ooops, I'll nuke it.

Ian.

Re: [PATCH] libxl: make libxl_cdrom_insert async

[Ian Campbell]

Somehow missed this comment ...

> > -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)
> >  {
> ...
> > +    if (!disk->pdev_path) {
> > +        disk->pdev_path = strdup("");
> 
> Don't you mean libxl__strdup ?  And presumably you should free the
> previous pdev_path ?

Previous pdev_path is NULL so there is no need to free it.

I didn't actually change this line, diff just thinks I did for some
reason, but I may as well make it a NOGC libxl__strdup while I'm here to
get the error handling.

> (TBH this massaging of the caller's libxl_device_disk is a bit
> unfriendly but I don't think getting rid of that is critical at this
> stage.  We can constify it later, in 4.3.)

It's quite a common pattern, the setdefaults function often does this
sort of thing. Actually, probably this should actually be in
libxl__device_disk_setdefault(), but lets not go there for 4.2.

Ian.

Re: [PATCH] libxl: make libxl_cdrom_insert async

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] libxl: make libxl_cdrom_insert async"):
> On Tue, 2012-07-24 at 17:49 +0100, Ian Jackson wrote:
> > This doesn't look actually /incorrect/ but I'm a bit suspicious: if
> > changing to a new non-empty disk, you first write params := "", in one
> > xenstore transaction, and then the non-empty value in a second
> > transaction.  Why ?
> 
> Mostly because xend did it that way, I agree that it seems pointless
> without adding any additional interlock to the protocol.
> 
> At one point I was hoping that copying xend closely would lead to
> working stubdom support etc, but that turned out to be a pipe dream.
> 
> Just writing the appropriate key once did seem to work, at least in the
> cases where it works at all, in my experiments so I guess I might as
> well change to that here.

If any other xenstore client depended on seeing the intermediate
value, the system would be buggy and it would only work by them
winning the race.  I guess it's possible that there would be a peer
with a strange bug which means that it misses the first watch event
but I think we should assume that there isn't unless we discover
otherwise.

Ian.

Re: [PATCH] libxl: make libxl_cdrom_insert async

[Ian Campbell]

On Wed, 2012-07-25 at 12:14 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] libxl: make libxl_cdrom_insert async"):
> > On Tue, 2012-07-24 at 17:49 +0100, Ian Jackson wrote:
> > > This doesn't look actually /incorrect/ but I'm a bit suspicious: if
> > > changing to a new non-empty disk, you first write params := "", in one
> > > xenstore transaction, and then the non-empty value in a second
> > > transaction.  Why ?
> > 
> > Mostly because xend did it that way, I agree that it seems pointless
> > without adding any additional interlock to the protocol.
> > 
> > At one point I was hoping that copying xend closely would lead to
> > working stubdom support etc, but that turned out to be a pipe dream.
> > 
> > Just writing the appropriate key once did seem to work, at least in the
> > cases where it works at all, in my experiments so I guess I might as
> > well change to that here.
> 
> If any other xenstore client depended on seeing the intermediate
> value, the system would be buggy and it would only work by them
> winning the race.  I guess it's possible that there would be a peer
> with a strange bug which means that it misses the first watch event
> but I think we should assume that there isn't unless we discover
> otherwise.

Agreed and changed in V2.

Ian.