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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list