Re: [RFC] xl disk configuration handling

[Kamala Narasimhan <kamala.narasimhan@gmail.com>]

Ian Campbell wrote:
> On Mon, 2011-01-31 at 17:18 +0000, Stefano Stabellini wrote: 
>> I agree that libxl_disk_phystype expresses both the format and the
>> backend type in a single confusing way, so there should be two enums:
>> one for the format (libxl_disk_format) and one for the backend type
>> (libxl_disk_pdev_type).
> 
> pdev_type doesn't seem like a very good name for "backend type" (and it
> is unnecessarily abbreviated which I personally dislike, especially in
> public interfaces).
> 
> Would libxl_disk_backend_type work?
>
Yes, I will go with libxl_disk_backend_type.

>>> diff -r 52e928af3637 tools/libxl/libxl.c
>>> --- a/tools/libxl/libxl.c     Fri Jan 28 19:37:49 2011 +0000
>>> +++ b/tools/libxl/libxl.c     Sun Jan 30 11:47:22 2011 -0500
>>> @@ -588,7 +588,7 @@ int libxl_wait_for_disk_ejects(libxl_ctx
>>>      for (i = 0; i < num_disks; i++) {
>>>          if (asprintf(&(waiter[i].path), "%s/device/vbd/%d/eject",
>>>                       libxl__xs_get_dompath(&gc, domid),
>>> -                     libxl__device_disk_dev_number(disks[i].virtpath)) < 0)
>>> +                     libxl__device_disk_dev_number(disks[i].vdev_path)) < 0)
>>>              goto out;
>>>          if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
>>>              goto out;
>> it would be nice if all the renaming was done in a separate patch so
>> that the real changes are easier to read
> 
> Aren't the renamings a bit cosmetic anyway, i.e. could/should be left
> for 4.2? I guess I can see the argument of changing the field name if
> the type name changes, assuming the type names are well chosen.
> 
I did consider keeping away from such renaming but then I figured I might as
well if I am going to make other changes to the interface.

> The virtpath->vdev_path rename in particular seems odd. The main thing
> which is wrong with virtpath is that the thing it contains is a vdev
> which doesn't really have any path-like properties, but we retain the
> path part in the new name. If it's renamed to anything I reckon it
> should just be vdev.
That makes sense.  I will rename it to just vdev.

Kamala