Re: [Xen-devel] [PATCH v10 07/19] libxl: convert libxl__device_disk_local_attach to an async op

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v10 07/19] libxl: convert 
libxl__device_disk_local_attach to an async op"):
> This will be needed in future patches, when libxl__device_disk_add
> becomes async also. Create a new status structure that defines the
> local attach of a disk device and use it in
> libxl__device_disk_local_attach.
> 
> This is done in this patch to split the changes introduced when
> libxl__device_disk_add becomes async.

Thanks for this.  See my other comment earlier today about the error
handling.  The rest of my comments are below.  Very nearly there I
think.

Thanks,
Ian.

> @@ -2234,39 +2237,102 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
>              goto out;
>      }
>      if (dev != NULL)
> -        ret = strdup(dev);
> -    return ret;
> +        dls->diskpath = strdup(dev);

libxl__strdup, surely.  And I'm not sure I understand why this string
can't be from the gc.  In any case the memory allocation strategy
should be documented in the struct.  Eg:

> +struct libxl__disk_local_state {
...
> +    /* filled by libxl__device_disk_local_initiate_attach */
> +    char *diskpath;

  +    char *diskpath; /* from the gc */
or
  +    char *diskpath; /* non-0 whenever Attached, disposed by detach */
or something.


> +void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
> +                                     libxl__disk_local_state *dls)
>  {
...
> +out:
> +    if (dls->diskpath) {
> +        free(dls->diskpath);
> +        dls->diskpath = 0;
> +    }
> +    /*
> +     * If there was an error in dls->rc, it means we have been called from
> +     * a failed execution of libxl__device_disk_local_initiate_attach,
> +     * so return the original error.
> +     */
> +    rc = dls->rc ? dls->rc : rc;
> +    dls->callback(egc, dls, rc);
> +    return;

This seems to occur twice.  Once here and once in
local_device_detach_cb.  Clearly they should be unified, probably by
dismembering local_device_detach_cb:

...
> +static void local_device_detach_cb(libxl__egc *egc, libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    libxl__disk_local_state *dls = CONTAINER_OF(aodev, *dls, aodev);
> +    int rc;
> +
> +    if (dls->diskpath) {
> +        free(dls->diskpath);
> +        dls->diskpath = 0;
> +    }
> +
> +    if (aodev->rc) {
...
> +    }
> +
> +out:
> +    /*
> +     * If there was an error in dls->rc, it means we have been called from
> +     * a failed execution of libxl__device_disk_local_initiate_attach,
> +     * so return the original error.
> +     */
> +    rc = dls->rc ? dls->rc : aodev->rc;
> +    dls->callback(egc, dls, rc);
> +    return;


> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index 7ebc0df..4bcca3d 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -249,10 +245,32 @@ static void bootloader_setpaths(libxl__gc *gc, 
> libxl__bootloader_state *bl)
...
> +/* Callbacks */
> +
> +static void bootloader_finished_cb(libxl__egc *egc,
> +                                   libxl__disk_local_state *dls,
> +                                   int rc);

Wouldn't this be better named
   bootloader_local_detached_cb
or something ?  Because the function called when the bootloader
finishes is bootloader_callback.

>  static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
>                                  int rc)
>  {
>      bootloader_cleanup(egc, bl);
> +
> +    bl->dls.callback = bootloader_finished_cb;
> +    libxl__device_disk_local_initiate_detach(egc, &bl->dls);
> +}

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4f3a232..ab590be 100644
...
> +/*
> + * Clears the internal error code, can be called multiple times, and
> + * must be called before libxl__device_disk_local_initiate_attach.
> + */
> +static inline void libxl__device_disk_local_init(libxl__disk_local_state 
> *dls)

"Clears the internal error code" should not be in interface
documentation, since it refers to an implementation detail.

Perhaps you mean "Prepares a dls for use".  You should explicitly
state which of the documented states it transitions between.  I guess
"Undefined -> Idle" ?


> +/* Make a disk available in this (the control) domain. Always calls
> + * dls->callback when finished.
> + * State Idle -> Attaching
> + *
> + * The state on which the device is when in the callback depends
> + * on the passed value of rc:
> + * Attached if rc == 0
> + * Idle if rc != 0

A nit: this would be slightly easier to read if there the two
subordinate options were indented.

Also correct English would be "the state in which the device is" but
really you mean the state of the dls, not of the device.  I would
write:
  + * The state of dls on entry to the callback depends on the value
  + * of rc passed to the callback:
  + *     rc == 0: Attached if rc == 0
  + *     rc != 0: Idle

> +_hidden void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
> +                                                libxl__disk_local_state 
> *dls);
> +
> +/* Disconnects a disk device form the control domain. If the passed
> + * dls is not attached (or has already been detached),
> + * libxl__device_disk_local_initiate_detach will just call the callback
> + * directly.
> + * State Idle/Attached -> Detaching
> + *
> + * The state on which the device is when in the callback is Idle.

Again, "in which", "dls" or reword as above.


> @@ -2097,10 +2142,10 @@ struct libxl__bootloader_state {
>      /* Should be zeroed by caller on entry.  Will be filled in by
>       * bootloader machinery; represents the local attachment of the
>       * disk for the benefit of the bootloader.  Must be detached by
> -     * the caller using libxl__device_disk_local_detach, but only
> +     * the caller using libxl__device_disk_local_initiate_detach, but only
>       * after the domain's kernel and initramfs have been loaded into
>       * memory and the file references disposed of. */

Is this last comment, which I wrote, wrong ?  That is, as previously
discussed, is it legal to detach the disk before the kernel and
initramfs have been loaded into the domain's memory ?

I think it probably is.  If so perhaps I should write a separate
patch to fix it.


Thanks,
Ian.

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