Re: [Xen-devel] [PATCH v6 02/11] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Stefano Stabellini writes ("[PATCH v6 02/11] libxl: 
libxl__device_disk_local_attach return a new libxl_device_disk"):
> Introduce a new libxl_device_disk* parameter to
> libxl__device_disk_local_attach, the parameter is allocated by the
> caller. libxl__device_disk_local_attach is going to fill the new disk
> with informations about the new locally attached disk.  The new
> libxl_device_disk should be passed to libxl_device_disk_local_detach
> afterwards.

In this declaration:

> @@ -1767,6 +1768,7 @@ struct libxl__bootloader_state {
>      libxl__bootloader_console_callback *console_available;
>      libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated 
> */
>      libxl_device_disk *disk;
> +    libxl_device_disk tmpdisk;
>      uint32_t domid;

We need information about what this "tmpdisk" is.  All of the other
parameters here are input parameters, except as otherwise noted in the
comment.

Also I'm not convinced that "tmpdisk" is quite the right name.  You
also need to explain the distinction between "disk" and "tmpdisk".

Perhaps:

   const libxl_device_disk *disk; /* as specified by user */
   libxl_device_disk localdisk;
      /* 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
       * after the domain's kernel and initramfs have been loaded into
       * memory and the file references disposed of. */

?

The implementation looks sane.

Ian.

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