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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> 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.

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