Re: [Xen-devel] [PATCH] libxl: QED disks support

[Juergen Gross <jgross@xxxxxxxx>]

On 08/12/16 10:28, Wei Liu wrote:
> On Wed, Dec 07, 2016 at 02:04:27PM +0100, Cedric Bosdonnat wrote:
>> On Wed, 2016-12-07 at 11:25 +0000, Wei Liu wrote:
>>> On Mon, Nov 14, 2016 at 03:57:00PM +0100, Cédric Bosdonnat wrote:
>>>> Qdisk supports qcow and qcow2, extend it to also support qed disk
>>>> format.
>>>>
>>>> Signed-off-by: Cédric Bosdonnat <cbosdonnat@xxxxxxxx>
>>>> ---
>>>>  tools/libxl/libxl_device.c  |    1 +
>>>>  tools/libxl/libxl_dm.c      |    1 +
>>>>  tools/libxl/libxl_types.idl |    1 +
>>>>  tools/libxl/libxl_utils.c   |    2 +
>>>>  tools/libxl/libxlu_disk_l.c | 1018 
>>>> ++++++++++++++++++++++---------------------
>>>>  tools/libxl/libxlu_disk_l.h |   53 +--
>>>>  tools/libxl/libxlu_disk_l.l |    3 +-
>>>
>>> You would also need to patch docs/misc/xl-disk-configuration.txt and
>>> possibly xl manpage.
>>
>> Oh indeed, I forgot about those.
>>
>>> You would also need to add a LIBXL_HAVE macro in libxl.h -- there are
>>> quite a lot of examples there.
>>
>> I pondered this too, but I managed to write a configure detection code 
>> detecting
>> if the format is managed without it (just by trying to build a small sample 
>> of code
>> using the new enum value). Do we really want to have a LIBXL_HAVE macro for 
>> every
>> single disk format support we add?
>>
> 
> The new macro is to mark the change in LIBXL public APIs, so that user
> can rely on the macro to do it, which seems to be a bit easier than
> requiring everyone to write a small program to test if the enum exists.
> 
> The macro just needs to carry a certain semantics. It doesn't have to be
> one macro per enum or thing. For example, you can add a batch of new
> formats but only have one macro. But in this particular patch, it is
> going to be one macro for this format.
> 
>>> Other than the things mentioned above, most code changes look rather
>>> mechanical to me.
>>>
>>> But what is not very satisfying (not your fault) is that we seem to need
>>> to add every single disk format we want to support by hand. That's
>>> rather repetitive. I wonder if there should be some sort of notation in
>>> libxl for "all formats that QEMU supports".
>>
>> Should I then check the docs for such a statement? Or should I try adding 
>> more
>> qemu-supported disk formats?
>>
> 
> Neither.
> 
> I was vaguely thinking about some sort of mechanism to automatically
> detect what QEMU supports and plumb relevant arguments to QEMU. But that
> doesn't seem to be easy and doesn't fit into our existing model. I think
> there will be security concern as well. We would need to think more
> about this.

Well, I've added indication of backend support of qemu via Xenstore. For
each supported backend type a Xenstore entry is being written. Adding
the supported formats under such a node seems to be a natural extension.


Juergen

> 
> At the moment I think your approach is fine. We just add new formats as
> they come along.
> 
> So, please update your patch to patch documents / libxl.h and resend.
> 
> Wei.
> 
>> --
>> Cedric
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
> 


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