Re: [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore

[Jim Fehlig <jfehlig@suse.com>]

Wei Liu wrote:
> On Tue, Feb 10, 2015 at 11:01:46AM +0000, Ian Jackson wrote:
>   
>> Wei Liu writes ("[PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore"):
>>     
>>> ... if backend is not set by caller.
>>>       
>> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>>
>> as far as it goes, but I think you may want a more radical change -
>> see below.
>>
>>     
>>> Also change the function to use "goto" idiom while I was there.
>>>       
>> (Although usually it would be better to split this kind of thing into
>> a pre-patch, in this case it's small and easily reviewed.)
>>
>> Is the backend type the only missing or potentially-wrong
>> information ?  ISTM that perhaps the caller might not know the target,
>> either.
>>
>> What should happen if the caller specifies a different target in disk
>> to the one the device is actually using ?  The documentation should
>> specify which of the fields are important.
>>
>>     
>
> I'm not sure because it's not documented.
>
> We should take a step back to define the important fields first.
>
>   
>> Maybe libxl_device_disk_remove needs to call libxl_vdev_to_device_disk
>> and check that the supplied disk struct is plausible somehow.  In that
>> case it might be nice for the caller to be able to fill in only the
>> vdev.
>>
>>     
>
> If so we need to make clear in the documentation. I'm of course fine
> with this behaviour.
>
> Jim, does libvirt (as an example of libxl user) actually cares
> specifying every fields in that struct? The other user (xl) doesn't seem
> to care that much.
>   

At minimum, libvirt will populate the pdev_path, vdev, backend, and
format fields. If backend and format (which, in libvirt-speack
correspond to the 'name' and 'type' attributes on the optional <driver>
element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
and LIBXL_DISK_FORMAT_RAW respectively.

Regards,
Jim