Re: [PATCH 5/6] libxl: introduce libxl__alloc_vdev

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Tue, 27 Mar 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH 5/6] libxl: introduce libxl__alloc_vdev"):
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> ...
> > +static int libxl__vdev_to_index(libxl__gc *gc, char *vdev)
> > +{
> > +    if (vdev == NULL)
> > +        return 0;
> > +    if (strlen(vdev) > 4 || strlen(vdev) < 4)
> > +        return 0;
> > +    /* assume xvdz is the last one available */
> 
> This is a broken half-reimplementation of
> libxl__device_disk_dev_number.

No it is not the same thing. This function doesn't return any meaningful
number representing the disk in a unique way, it basically returns the last
letter of the disk name as an integer (minus offset).


> > +    free(disks);
> > +    return libxl__index_to_vdev(gc, max_idx);
> 
> And you don't need this function because you can use the disk number
> directly (converted to a string of decimal digits) as the vdev.

These two functions are not the same as libxl__device_disk_dev_number
and friends. They are just helper functions for libxl__alloc_vdev.
I could rename them libxl__vdev_helper_idx and libxl__vdev_helper_name
to avoid confusion.


> Of course you can't then using the resulting vdev as a pathname in
> /dev/ but that's nonportable anyway; different guest OSs (and here we
> are playing the role of the guest) may have different conventions.
> You need a separate function to convert vdev numbers (as returned from
> _disk_dev_number) to pathnames in /dev.

This is a good point and the key issue here.

The frontend is actually free to choose whatever name it wants for the
device it is going to create from the dev number.
We don't have a proper way to get this device name, the closest thing we
have is libxl__device_disk_dev_number that from a vdev returns the dev
number.
So I think that the best thing we can do is rely on it to find out the
device name, either doing:

[1] xvd(a + i) -> libxl__device_disk_dev_number -> xs_read

or reading the "dev" node under the xenstore backend path, that is
generated using the same libxl function (this is what I am doing now).
I fail to see why one of these two approaches would be better than the
other, but if you think that [1] is the best, I can change my code.

 
> > +    for (i = 0; i < num; i++) {
> > +        idx = libxl__vdev_to_index(gc, disks[i].vdev);
> 
> Furthermore, for the purpose for which you're using this, you should
> not start at zero (xvda).  You should start at at least 26 (xvdA) and
> possibly later.  That way you are less likely to clash with other
> statically-allocated vbds.
 
Another interesting observation but I disagree: why should we expect
"statically-allocated vbds" below xvdA? How many do you think we are
going to find? Why 26? I think it is very arbitrary.
We should just make sure that our allocation policy works well and start
from 0.