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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home