From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [RFC] xl disk configuration handling Date: Mon, 31 Jan 2011 19:25:57 +0000 Message-ID: <1296501957.20804.198.camel@localhost.localdomain> References: <4D45A410.4000304@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Stefano Stabellini Cc: Kamala Narasimhan , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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.