Re: [PATCH] libxl: QED disks support

[Wei Liu <wei.liu2@citrix.com>]

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@suse.com>
> > > ---
> > >  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.

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@lists.xen.org
> https://lists.xen.org/xen-devel

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