Re: [RFC] xl disk configuration handling

[Ian Campbell <Ian.Campbell@citrix.com>]

On Mon, 2011-01-31 at 17:18 +0000, Stefano Stabellini wrote: 
> On Sun, 30 Jan 2011, Kamala Narasimhan wrote:
> > diff -r 52e928af3637 tools/libxl/libxl.h
> > --- a/tools/libxl/libxl.h     Fri Jan 28 19:37:49 2011 +0000
> > +++ b/tools/libxl/libxl.h     Sun Jan 30 11:47:22 2011 -0500
> > @@ -172,14 +172,27 @@ typedef enum {
> >  } libxl_console_consback;
> >
> >  typedef enum {
> > -    PHYSTYPE_QCOW = 1,
> > -    PHYSTYPE_QCOW2,
> > -    PHYSTYPE_VHD,
> > -    PHYSTYPE_AIO,
> > -    PHYSTYPE_FILE,
> > -    PHYSTYPE_PHY,
> > -    PHYSTYPE_EMPTY,
> > -} libxl_disk_phystype;
> > +    DISK_FORMAT_UNKNOWN = 0,
> > +    DISK_FORMAT_QCOW,
> > +    DISK_FORMAT_QCOW2,
> > +    DISK_FORMAT_VHD,
> > +    DISK_FORMAT_RAW,
> > +} libxl_disk_format;
> > +
> > +typedef enum {
> > +    DISK_IMPL_TYPE_UNKNOWN = 0,
> > +    DISK_IMPL_TYPE_AIO,
> > +    DISK_IMPL_TYPE_TAPDISK,
> > +    DISK_IMPL_TYPE_IOEMU,
> > +} libxl_disk_impl_type;
> > +
> > +typedef enum {
> > +    DISK_PDEV_TYPE_UNKNOWN = 0,
> > +    DISK_PDEV_TYPE_PHY,
> > +    DISK_PDEV_TYPE_FILE,
> > +    DISK_PDEV_TYPE_TAP,
> > +    DISK_PDEV_TYPE_TAP2,
> > +} libxl_disk_pdev_type;
> >
> >  typedef enum {
> >      NICTYPE_IOEMU = 1,
> 
> I agree that libxl_disk_phystype expresses both the format and the
> backend type in a single confusing way, so there should be two enums:
> one for the format (libxl_disk_format) and one for the backend type
> (libxl_disk_pdev_type).

pdev_type doesn't seem like a very good name for "backend type" (and it
is unnecessarily abbreviated which I personally dislike, especially in
public interfaces).

Would libxl_disk_backend_type work?

> > diff -r 52e928af3637 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c     Fri Jan 28 19:37:49 2011 +0000
> > +++ b/tools/libxl/libxl.c     Sun Jan 30 11:47:22 2011 -0500
> > @@ -588,7 +588,7 @@ int libxl_wait_for_disk_ejects(libxl_ctx
> >      for (i = 0; i < num_disks; i++) {
> >          if (asprintf(&(waiter[i].path), "%s/device/vbd/%d/eject",
> >                       libxl__xs_get_dompath(&gc, domid),
> > -                     libxl__device_disk_dev_number(disks[i].virtpath)) < 0)
> > +                     libxl__device_disk_dev_number(disks[i].vdev_path)) < 0)
> >              goto out;
> >          if (asprintf(&(waiter[i].token), "%d", LIBXL_EVENT_DISK_EJECT) < 0)
> >              goto out;
> 
> it would be nice if all the renaming was done in a separate patch so
> that the real changes are easier to read

Aren't the renamings a bit cosmetic anyway, i.e. could/should be left
for 4.2? I guess I can see the argument of changing the field name if
the type name changes, assuming the type names are well chosen.

The virtpath->vdev_path rename in particular seems odd. The main thing
which is wrong with virtpath is that the thing it contains is a vdev
which doesn't really have any path-like properties, but we retain the
path part in the new name. If it's renamed to anything I reckon it
should just be vdev.

Ian.