Re: [Xen-devel] [PATCH v11 03/17] libxl: convert libxl__device_disk_local_attach to an async op

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v11 03/17] 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.

Thanks.  Close...

> +void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
> +                                     libxl__disk_local_state *dls)
>  {
> +    STATE_AO_GC(dls->ao);
>      int rc = 0;
> +    libxl_device_disk *disk = &dls->disk;
> +    libxl__device *device;
> +    libxl__ao_device *aodev = &dls->aodev;
> +    libxl__prepare_ao_device(ao, aodev);
> +
> +    if (!dls->diskpath) goto out;
> 
>      switch (disk->backend) {
>          case LIBXL_DISK_BACKEND_QDISK:
...
> +                rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID,
> +                                             disk, device);
> +                if (rc != 0) goto out;
...
>          default:
>              /*
>               * Nothing to do for PHYSTYPE_PHY.
>               * For other device types assume that the blktap2 process is
>               * needed by the soon to be started domain and do nothing.
>               */
> -            break;
> +            goto out;
>      }
> 
> +out:
> +    local_device_detach_cb(egc, aodev);

Doesn't this lose the rc ?  Since...

...
> +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 (aodev->rc) {

... this picks the rc out of the aodev.  And you don't set aodev->rc.
The general code in libxl__device_disk_local_initiate_detach expects
(as is conventional) to set the local variable rc and `goto out'.  So
I think you need
  +    aodev->rc = rc;

> @@ -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. */

I queried this and you replied:
> I didn't change this comment, but if you want I can do that in this
> patch. The bootloader makes a copy of the kernel/initramfs to somewhere,
> and that is what we load to the memory.

Right.  The comment is wrong, but you're not making it wronger, so
this is something for me to fix up.

Ian.

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