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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

Or couldn't it actually be an int and save you parsing at all?

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

I'd be tempted to inline the GCSPRINTF in the arg and do away with vdev
since you don't use it again.

> +        if (libxl__xs_read(gc, t,
> +                    libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> +                        dompath, devid)) == NULL) {
> +            if (errno == ENOENT)
> +                return libxl__devid_to_localdev(gc, devid);
> +            else
> +                return NULL;
> +        }
> +        disk++;
> +    } while (disk <= (1 << 20));

That's a whole lotta disks ;-)

> +    return NULL;
> +}
> +
>  char * libxl__device_disk_local_attach(libxl__gc *gc,
>          const libxl_device_disk *in_disk,
>          libxl_device_disk **new_disk,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 0074ec4..a451c79 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -77,6 +77,8 @@
>  #define LIBXL_PV_EXTRA_MEMORY 1024
>  #define LIBXL_HVM_EXTRA_MEMORY 2048
>  #define LIBXL_MIN_DOM0_MEM (128*1024)
> +/* use 0 as the domid of the toolstack domain for now */
> +#define LIBXL_TOOLSTACK_DOMID 0
>  #define QEMU_SIGNATURE "DeviceModelRecord0002"
>  #define STUBDOM_CONSOLE_LOGGING 0
>  #define STUBDOM_CONSOLE_SAVE 1
> @@ -763,6 +765,8 @@ _hidden int libxl__ev_devstate_wait(libxl__gc *gc, 
> libxl__ev_devstate *ds,
>   */
>  _hidden int libxl__try_phy_backend(mode_t st_mode);
>  
> +_hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
> +
>  /* from libxl_pci */
>  
>  _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, 
> libxl_device_pci *pcidev, int starting);
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 925248b..cb596a9 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -25,3 +25,48 @@ int libxl__try_phy_backend(mode_t st_mode)
>  
>      return 1;
>  }
> +
> +#define EXT_SHIFT 28
> +#define EXTENDED (1<<EXT_SHIFT)
> +#define VDEV_IS_EXTENDED(dev) ((dev)&(EXTENDED))
> +#define BLKIF_MINOR_EXT(dev) ((dev)&(~EXTENDED))
> +
> +/* same as in Linux */
> +static char *encode_disk_name(char *ptr, unsigned int n)
> +{
> +    if (n >= 26)
> +        ptr = encode_disk_name(ptr, n / 26 - 1);
> +    *ptr = 'a' + n % 26;
> +    return ptr + 1;
> +}
> +
> +char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
> +{
> +    int minor;
> +    int offset;
> +    int nr_parts;
> +    char *ptr = NULL;
> +    char *ret = libxl__zalloc(gc, 32);
> +
> +    if (!VDEV_IS_EXTENDED(devid)) {
> +        minor = devid & 0xff;
> +        nr_parts = 16;
> +    } else {
> +        minor = BLKIF_MINOR_EXT(devid);
> +        nr_parts = 256;
> +    }
> +    offset = minor / nr_parts;
> +
> +     /* arbitrary upper bound */
> +     if (offset > 676)
> +             return NULL;
> +
> +    strcpy(ret, "xvd");
> +    ptr = encode_disk_name(ret + 3, offset);
> +    if (minor % nr_parts == 0)
> +        *ptr = 0;
> +    else
> +        snprintf(ptr, ret + 32 - ptr,
> +                "%d", minor & (nr_parts - 1));
> +    return ret;
> +}
> 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?

> @@ -24,3 +24,9 @@ int libxl__try_phy_backend(mode_t st_mode)
>  
>      return 0;
>  }
> +
> +char *libxl__devid_to_localdev(libxl__gc *gc, int devid)
> +{
> +    /* TODO */
> +    return NULL;
> +}



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