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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Stefano Stabellini writes ("[PATCH v7 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.

Thanks.

So comparing the comment:

> +    /* 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. */
> +    libxl_device_disk localdisk;

with the code in the caller: on entry it is indeed zeroed by the GCNEW
of the whole domain_create_state.  However on exit:

>      if (bl->diskpath) {
> -        libxl__device_disk_local_detach(gc, bl->disk);
> +        libxl__device_disk_local_detach(gc, &bl->localdisk);
> +        free(bl->localdisk.pdev_path);
> +        free(bl->localdisk.script);
>          free(bl->diskpath);

This does not correspond exactly to the comment.  Not only do we call
detach but apparently we need to free some things too.

It's not clear to me why these things haven't come from a suitable
gc.  But if they can't come from a gc then the comment needs to
mention that they need to be freed.

> -char * libxl__device_disk_local_attach(libxl__gc *gc, libxl_device_disk 
> *disk)
> +char * libxl__device_disk_local_attach(libxl__gc *gc,
> +        const libxl_device_disk *in_disk,
> +        libxl_device_disk *disk)
>  {
>      libxl_ctx *ctx = gc->owner;
>      char *dev = NULL;
>      char *ret = NULL;
>      int rc;
>  
> +    if (in_disk->pdev_path == NULL)
> +        return NULL;
> +
> +    memcpy(disk, in_disk, sizeof(libxl_device_disk));
> +    disk->pdev_path = strdup(in_disk->pdev_path);
> +    if (in_disk->script != NULL)
> +        disk->script = strdup(in_disk->script);
> +    disk->vdev = NULL;

Re my comment about the gc, do we call this function anywhere else ?
Could it take the ao for the benefit of its gc ?  Would that violate
some rules about the usual contents of a libxl_device_disk ?

> @@ -367,8 +378,8 @@ char * libxl__device_disk_local_attach(libxl__gc *gc, 
> libxl_device_disk *disk)
>                             " attach a qdisk image if the format is not raw");
>                  break;
>              }
> -            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> -                       disk->pdev_path);
> +            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s",
> +                       in_disk->pdev_path);

A very minor comment, but: you might want to change this to LOG while
you're at it.

Thanks,
Ian.

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