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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> 
> 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.

Wei.

> Ian.

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