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

[Roger Pau Monne <roger.pau@citrix.com>]

Ian Jackson wrote:
> 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.

I've fixed all the above, thanks for the review.