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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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?


> > +    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'd be tempted to inline the GCSPRINTF in the arg and do away with vdev
> since you don't use it again.

OK 

> > 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.

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