Re: [Xen-devel] [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-05-14 at 14:48 +0100, Stefano Stabellini wrote:
> On Fri, 4 May 2012, Ian Campbell wrote:
> > On Fri, 2012-05-04 at 12:13 +0100, Stefano Stabellini wrote:
> > > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > > domain passed as argument.
> > > 
> > > Changes in v5:
> > > - remove domid paramter to libxl__alloc_vdev (assume
> > >   LIBXL_TOOLSTACK_DOMID);
> > > - remove scaling limit from libxl__alloc_vdev.
> > > 
> > > Changes in v4:
> > > - rename libxl__devid_to_vdev to libxl__devid_to_localdev;
> > > - introduce upper bound for encode_disk_name;
> > > - better error handling;
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > ---
> > >  tools/libxl/libxl_internal.c |   31 ++++++++++++++++++++++++++++
> > >  tools/libxl/libxl_internal.h |    4 +++
> > >  tools/libxl/libxl_linux.c    |   45 
> > > ++++++++++++++++++++++++++++++++++++++++++
> > >  tools/libxl/libxl_netbsd.c   |    6 +++++
> > >  4 files changed, 86 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > > index bd5ebb3..7a1e017 100644
> > > --- a/tools/libxl/libxl_internal.c
> > > +++ b/tools/libxl/libxl_internal.c
> > > @@ -480,6 +480,37 @@ out:
> > >      return rc;
> > >  }
> > >  
> > > +/* libxl__alloc_vdev only works on the local domain, that is the domain
> > > + * where the toolstack is running */
> > > +static char * libxl__alloc_vdev(libxl__gc *gc, const char *blkdev_start,
> > > +        xs_transaction_t t)
> > > +{
> > > +    int devid = 0, disk = 0, part = 0;
> > > +    char *vdev = NULL;
> > > +    char *dompath = libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID);
> > > +
> > > +    libxl__device_disk_dev_number(blkdev_start, &disk, &part);
> > 
> > If you specify the default blkdev_start in xl as d0 instead of xvda
> > doesn't at least this bit magically become portable to BSD etc too?
> 
> It cannot work on BSD for other reason, see below.
> In any case blkdev_start can be anything that
> libxl__device_disk_dev_number can parse, so d0p0 works too.
> 
> 
> > Or couldn't it actually be an int and save you parsing at all?
> 
> Yes, it could. But isn't "xvda" much more intuitive?

To Linux users, yes. But on BSD the virt devices have a different naming
scheme, so xvda wouldn't necessarily make much sense to them.

However I agree with IanJ's comment that letting the user use a name is
friendlier than just a bare number.

Anyway, lets just leave it as "xvda", it's not a huge deal.

> > > +    if (part != 0) {
> > > +        LOG(ERROR, "blkdev_start is invalid");
> > > +        return NULL;
> > > +    }
> > > +
> > > +    do {
> > > +        vdev = GCSPRINTF("d%dp0", disk);
> > > +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> > 
> > libxl__device_disk_dev_number does the right thing with "d<N>" (i.e.
> > without the hardcoded "p0"), I think.
> 
> In my tests it doesn't work properly using just d<N>.

I misread it, the sscanf in libxl__device_disk_dev_number needs to match
at least twice (so "dN" and "pM"). In theory that could be changed but
lets not do that now.

> > > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> > > index 9e0ed6d..dbf5f71 100644
> > > --- a/tools/libxl/libxl_netbsd.c
> > > +++ b/tools/libxl/libxl_netbsd.c
> > 
> > Aha, here's where we need a BSD contribution. CC someone?
> 
> There is no working gntdev or QDISK on BSD at the moment.

Oh, right, that does make it somewhat pointless...

Ian.



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