Re: [Xen-devel] [PATCH v9 05/17] libxl: refactor disk addition to take a helper

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v9 05/17] libxl: refactor disk addition to take 
a helper"):
> Change libxl__device_disk_add to no longer take a xs transaction and
> instead pass a helper for the local attach case that's used to get the
> free vdev.
...> 
> This function contains some non-functional changes due to an
> indentation change.

> +/* Specific function called directly only by local disk attach,
> + * all other users should instead use the regular
> + * libxl__device_disk_add wrapper
> + */
> +static int device_disk_add(libxl__gc *gc, uint32_t domid,
> +                           libxl_device_disk *disk,
> +                           char *(*fn)(libxl__gc *, const char *,
> +                                              xs_transaction_t),
> +                           const char *blkdev_start)

(I'm going to be quoting a diff -b.)

A better comment would be one which described what `fn' does.  Ie,
rather than saying `this is internal to local attach', describe its
semantics.

Maybe also `fn' could have a better name.  `get_vdev' ?

And the context pointer should be a void*, not a const char*.
`void *get_vdev_user' or something.

...
> +        if (fn) {
> +            assert(blkdev_start);
> +            disk->vdev = fn(gc, blkdev_start, t);
> +            if (disk->vdev == NULL) {
> +                LOG(ERROR, "libxl__alloc_vdev failed");

Surely this logging is (a) not necessarily true since the caller's fn
may have nothing to do with libxl__alloc_vdev (b) should be done by fn
anyway, since fn probably knows what is going on.

> +        if (front)
> +            flexarray_free(front);
>          front = flexarray_make(16, 1);

Urgh, this is a bit unpleasant, isn't it.  I can't see a better way to
do it though.

Thanks,
Ian.

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