Re: [Xen-devel] [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore
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
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.
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.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore
On Tue, Feb 10, 2015 at 11:01:46AM +, 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
>
> 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@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore
Wei Liu wrote:
> On Tue, Feb 10, 2015 at 11:01:46AM +, 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
>>
>> 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
element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
and LIBXL_DISK_FORMAT_RAW respectively.
Regards,
Jim
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore
Jim Fehlig writes ("Re: [PATCH 3/3] libxl: libxl__device_from_disk should
retrieve backend from xenstore"):
> Wei Liu wrote:
> > On Tue, Feb 10, 2015 at 11:01:46AM +, Ian Jackson wrote:
> >> 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.
I know :-). I meant: what should we write in the documentation ?
(And, obviously, implement.)
> > We should take a step back to define the important fields first.
Right.
> >> 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.
Well, feel free to object if you think my (rather vague) suggestion is
wrong.
> > 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
> element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
> and LIBXL_DISK_FORMAT_RAW respectively.
I think for fields libvirt has gone to the trouble of specifying,
libxl should check that they match.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore
On Wed, Feb 11, 2015 at 10:18:18AM -0700, Jim Fehlig wrote:
> Wei Liu wrote:
> > On Tue, Feb 10, 2015 at 11:01:46AM +, 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
> >>
> >> 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
> element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN
> and LIBXL_DISK_FORMAT_RAW respectively.
>
Since libvirt has a tendency of specifying everything, how come there is
no "name" and "type" in ? Can we actually generate all the
fields needed when attaching a disk and store in libvirt's diskspec?
Wei.
> Regards,
> Jim
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore
Wei Liu wrote: > On Wed, Feb 11, 2015 at 10:18:18AM -0700, Jim Fehlig wrote: > >> 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 >> element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN >> and LIBXL_DISK_FORMAT_RAW respectively. >> >> > > Since libvirt has a tendency of specifying everything, how come there is > no "name" and "type" in ? The element is optional. From http://libvirt.org/formatdomain.html#elementsDisks "|driver: |The optional driver element allows specifying further details related to the hypervisor driver used to provide the disk" And when not specified, Ian C. recommended allowing libxl to pick suitable defaults https://www.redhat.com/archives/libvir-list/2013-February/msg01126.html > Can we actually generate all the > > fields needed when attaching a disk and store in libvirt's diskspec? Yes, it was this way before the suggested change. Regards, Jim ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] libxl: libxl__device_from_disk should retrieve backend from xenstore
Sorry for the late reply. On Thu, Feb 26, 2015 at 04:49:21PM -0700, Jim Fehlig wrote: > Wei Liu wrote: > > On Wed, Feb 11, 2015 at 10:18:18AM -0700, Jim Fehlig wrote: > > > >> 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 > >> element) are not specified, they are set to LIBXL_DISK_BACKEND_UNKNOWN > >> and LIBXL_DISK_FORMAT_RAW respectively. > >> > >> > > > > Since libvirt has a tendency of specifying everything, how come there is > > no "name" and "type" in ? > > The element is optional. From > http://libvirt.org/formatdomain.html#elementsDisks > > "|driver: |The optional driver element allows specifying further details > related to the hypervisor driver used to provide the disk" > > And when not specified, Ian C. recommended allowing libxl to pick > suitable defaults > > https://www.redhat.com/archives/libvir-list/2013-February/msg01126.html > > > Can we actually generate all the > > > > fields needed when attaching a disk and store in libvirt's diskspec? > > Yes, it was this way before the suggested change. > I'm now of the opinion that we shouldn't change libxl__device_from_disk to fill in specific information, otherwise it becomes a special case for all the libxl__device_from_$foo functions. And from the information you provide above it seems to be easily fixable on libvirt side -- you have all the information at hand when attaching the disk, so why not just store it for reuse later? Ian C, what do you think? Wei. > Regards, > Jim ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
