[libvirt] [PATCH]: Rework xenLinuxDomainDeviceID to support more devices

[Daniel P. Berrange]

On Tue, Aug 05, 2008 at 12:12:21PM +0200, Chris Lalancette wrote:
> Recently upstream Xen added support for having xvd devices > 16.  For the most
> part, this doesn't really concern libvirt, since for things like attach and
> detach we just pass it through and let xend worry about whether it is supported
> or not.  The one place this breaks down is in the stats collecting code, where
> we need to figure out the device number so we can go digging in /sys for the
> statistics.
> 
> To remedy this, I've re-written xenLinuxDomainDeviceID() to use regular
> expressions to figure out the device number from the name.  The major advantage
> is that now xenLinuxDomainDeviceID() looks fairly identical to
> tools/python/xen/util/blkif.py (in the Xen sources), so that adding additional
> devices in the future should be much easier.  It also reduces the size of the
> code, and, in my opinion, the code complexity.
> 
> With this patch in place, I was able to get block statistics both on older style
> devices (/dev/xvda) and on the new, expanded devices (/dev/xvdaa).

This patch breaks the test suite for disk name -> device ID conversion.
The test suite also needs to have more  tests added to cover the new 
interesting boundary conditions for xvdXXX naming.

>  
> -    if (strlen (path) >= 4 &&
> -        STRPREFIX (path, "xvd")) {
> -        /* Xen paravirt device handling */
> -        disk = (path[3] - 'a');
> -        if (disk < 0 || disk > 15) {
> -            statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
> -                            "invalid path, device names must be in range xvda - xvdp",
> -                            domid);
> -            return -1;
> -        }

I don't like the fact that for 'out of range' values this patch is removing
this accurate and helpful error message.....

> -        return (majors[disk/2] * 256) + ((disk % 2) * 63) + part;
> +    if (disk_re_match("/dev/sd[a-z]([1-9]|1[0-5])?$", mod_path, &part)) {
> +        major = scsi_majors[(mod_path[7] - 'a') / 16];
> +        minor = ((mod_path[7] - 'a') % 16) * 16 + part;
> +        retval = major * 256 + minor;
>      }
> +    else if (disk_re_match("/dev/sd[a-i][a-z]([1-9]|1[0-5])?$",
> +                           mod_path, &part)) {
> +        major = scsi_majors[((mod_path[7] - 'a' + 1) * 26 + (mod_path[8] - 'a')) / 16];
> +        minor = (((mod_path[7] -'a' + 1) * 26 + (mod_path[8] - 'a')) % 16)
> +            * 16 + part;
> +        retval = major * 256 + minor;
> +    }
> +    else if (disk_re_match("/dev/hd[a-t]([1-9]|[1-5][0-9]|6[0-3])?",
> +                           mod_path, &part)) {
> +        major = ide_majors[(mod_path[7] - 'a') / 2];
> +        minor = ((mod_path[7] - 'a') % 2) * 64 + part;
> +        retval = major * 256 + minor;
> +    }
> +    else if (disk_re_match("/dev/xvd[a-p]([1-9]|1[0-5])?$", mod_path, &part))
> +        retval = (202 << 8) + ((mod_path[8] - 'a') << 4) + part;
> +    else if (disk_re_match("/dev/xvd[q-z]([1-9]|1[0-5])?$", mod_path, &part))
> +        retval = (1 << 28) + ((mod_path[8] - 'a') << 8) + part;
> +    else if (disk_re_match("/dev/xvd[a-i][a-z]([1-9]|1[0-5])?$",
> +                           mod_path, &part))
> +        retval = (1 << 28) + (((mod_path[8] - 'a' + 1) * 26 + (mod_path[9] - 'a')) << 8) + part;
> +    else
> +        statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
> +                        "unsupported path, use xvdN, hdN, or sdN", domid);

...and replacing it with this unhelpful & misleading one. 

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|