[libvirt] [PATCH]: Rework xenLinuxDomainDeviceID to support more devices
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). Signed-off-by: Chris Lalancette <clalance@redhat.com>
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 :|
Daniel P. Berrange wrote:
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.
OK. Well, there were 3 different problems with the test suite: 1) A number of tests were actually wrong. For instance, there is a DO_TEST("/dev/hdt", 23359); but Xen actually uses the encoding "major*256 + minor + part". So in this case, the major is 91, and the minor is 64 (according to http://www.lanana.org/docs/device-list/devices.txt), so that would be 23360. I've fixed the wrong tests now, and I'll re-submit it with the updated patch. 2) In the hda0 and hda64 case, the upstream Xen regex isn't tight enough. I've tightened it up in the libvirt patch, so these now pass. 3) The upstream Xen regex's allows /dev/sdi{w,x,y,z}, although they aren't legal devices. I've fixed up my regex to handle this. Attached is an updated patch with the above fixes both to my code and to the test suite. As far as the error reporting goes, I won't argue that my patch gives slightly less information. However, that being said, I have to believe that the most likely use of block statistics is something like: virsh dumpxml <dom> ...see what devices are listed there virsh domblkstats <dom> <device> In which case the slightly less verbose error reporting won't matter a whole lot. Chris Lalancette
On Tue, Aug 05, 2008 at 02:08:24PM +0200, Chris Lalancette wrote:
Daniel P. Berrange wrote:
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.
OK. Well, there were 3 different problems with the test suite:
1) A number of tests were actually wrong. For instance, there is a DO_TEST("/dev/hdt", 23359); but Xen actually uses the encoding "major*256 + minor + part". So in this case, the major is 91, and the minor is 64 (according to http://www.lanana.org/docs/device-list/devices.txt), so that would be 23360. I've fixed the wrong tests now, and I'll re-submit it with the updated patch.
2) In the hda0 and hda64 case, the upstream Xen regex isn't tight enough. I've tightened it up in the libvirt patch, so these now pass.
3) The upstream Xen regex's allows /dev/sdi{w,x,y,z}, although they aren't legal devices. I've fixed up my regex to handle this.
Attached is an updated patch with the above fixes both to my code and to the test suite. As far as the error reporting goes, I won't argue that my patch gives slightly less information. However, that being said, I have to believe that the most likely use of block statistics is something like:
I'd like to see a patch which does an hybrid of the existing vs regex approach. ie, use a single regex to split the string into disk prefix, disk number, and partition number. Then do validation on the ranges and report errors accordingly.
virsh dumpxml <dom> ...see what devices are listed there virsh domblkstats <dom> <device>
In which case the slightly less verbose error reporting won't matter a whole lot.
Error reporting shouldn't be done based on the based on a particular usage scenario. Any API call should aim to give error messages that are detailed enough to understand the actual problem without needing further context. 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 :|
Chris Lalancette <clalance@redhat.com> wrote: ...
test suite. As far as the error reporting goes, I won't argue that my patch gives slightly less information. However, that being said, I have to believe that the most likely use of block statistics is something like:
virsh dumpxml <dom> ...see what devices are listed there virsh domblkstats <dom> <device>
In which case the slightly less verbose error reporting won't matter a whole lot.
Hi Chris, ...
Index: src/stats_linux.c =================================================================== ... +static int +disk_re_match(const char *regex, const char *path, int *part) +{ + regex_t myreg; + int err; + int retval; + regmatch_t pmatch[2]; + + retval = 0; + + err = regcomp(&myreg, regex, REG_EXTENDED); + if (err != 0) + return 0; + + err = regexec(&myreg, path, 2, pmatch, 0); + + if (err == 0) { + /* OK, we have a match; see if we have a partition */ + *part = 0; + if (pmatch[1].rm_so != -1) { + if (virStrToLong_i(path + pmatch[1].rm_so, NULL, 10, part) < 0) + retval = 0; + else + retval = 1; + } + else + retval = 1; + }
How about dropping both "else" blocks and initializing "retval=1" above: if (err == 0) { /* OK, we have a match; see if we have a partition */ *part = 0; retval = 1; if (pmatch[1].rm_so != -1) { if (virStrToLong_i(path + pmatch[1].rm_so, NULL, 10, part) < 0) retval = 0; } } You could even reduce the retval-setting part to a single statement: if (err == 0) { /* OK, we have a match; see if we have a partition */ *part = 0; retval = (pmatch[1].rm_so == -1 || virStrToLong_i(path + pmatch[1].rm_so, NULL, 10, part) == 0); }
int xenLinuxDomainDeviceID(virConnectPtr conn, int domid, const char *path) { - int disk, part = 0; - - /* Strip leading path if any */ - if (strlen(path) > 5 && - STRPREFIX(path, "/dev/")) - path += 5; + int major, minor; + int part; + int retval; + char *mod_path; + + int scsi_majors[] = { SCSI_DISK0_MAJOR, SCSI_DISK1_MAJOR,
I know you just moved these, but they can be const: int const scsi_majors[] = {... ...
+ int ide_majors[] = { IDE0_MAJOR, IDE1_MAJOR, IDE2_MAJOR, IDE3_MAJOR,
int const ide_majors[] = { ... ...
+ if (strlen(path) > 5 && STRPREFIX(path, "/dev/"))
Maybe use ">= 5", so that this code will preserve /dev/, rather than turn it into "/dev//dev/" ?
+ retval = asprintf(&mod_path, "%s", path); + else + retval = asprintf(&mod_path, "/dev/%s", path); ... + 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-h][a-z]([1-9]|1[0-5])?$", + mod_path, &part) || + disk_re_match("/dev/sdi[a-v]([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;
To retain the diagnostic Dan mentioned, you should be able to insert something like this just before the final "else": else if (disk_re_match("/dev/sd[a-z]([0-9])+$", mod_path, &part)) {
+ else + statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__, + "unsupported path, use xvdN, hdN, or sdN", domid);
Jim Meyering wrote:
To retain the diagnostic Dan mentioned, you should be able to insert something like this just before the final "else":
else if (disk_re_match("/dev/sd[a-z]([0-9])+$", mod_path, &part)) {
+ else + statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__, + "unsupported path, use xvdN, hdN, or sdN", domid);
OK, another go, with all of Jim's concerns addressed. I did something like this last point (thanks for the idea Jim), except that I didn't use regex's but basic STRPREFIX() to get better error messages. Dan, is this better? Chris Lalancette
Chris Lalancette <clalance@redhat.com> wrote: ..
OK, another go, with all of Jim's concerns addressed. I did something like this last point (thanks for the idea Jim), except that I didn't use regex's but basic STRPREFIX() to get better error messages.
Good idea. No point in using a regexp just to match a prefix. ACK.
On Tue, Aug 05, 2008 at 03:45:40PM +0200, Chris Lalancette wrote:
Jim Meyering wrote:
To retain the diagnostic Dan mentioned, you should be able to insert something like this just before the final "else":
else if (disk_re_match("/dev/sd[a-z]([0-9])+$", mod_path, &part)) {
+ else + statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__, + "unsupported path, use xvdN, hdN, or sdN", domid);
OK, another go, with all of Jim's concerns addressed. I did something like this last point (thanks for the idea Jim), except that I didn't use regex's but basic STRPREFIX() to get better error messages. Dan, is this better?
ACK, this gets my vote now. Regards, 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 :|
Chris Lalancette wrote:
Jim Meyering wrote:
To retain the diagnostic Dan mentioned, you should be able to insert something like this just before the final "else":
else if (disk_re_match("/dev/sd[a-z]([0-9])+$", mod_path, &part)) {
+ else + statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__, + "unsupported path, use xvdN, hdN, or sdN", domid);
OK, another go, with all of Jim's concerns addressed. I did something like this last point (thanks for the idea Jim), except that I didn't use regex's but basic STRPREFIX() to get better error messages. Dan, is this better?
Committed this version. Chris Lalancette
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).
Sounds good to me even if I don't grasp all the naming convention.
Signed-off-by: Chris Lalancette <clalance@redhat.com> [...] + /* OK, we have a match; see if we have a partition */ + *part = 0; + if (pmatch[1].rm_so != -1) + *part = strtol(path + pmatch[1].rm_so, NULL, 10);
let's use __virStrToLong_i internally I think it makes that code a bit more readable too, +1 thanks Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Tue, Aug 05, 2008 at 06:47:34AM -0400, Daniel Veillard wrote:
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).
Sounds good to me even if I don't grasp all the naming convention.
Signed-off-by: Chris Lalancette <clalance@redhat.com> [...] + /* OK, we have a match; see if we have a partition */ + *part = 0; + if (pmatch[1].rm_so != -1) + *part = strtol(path + pmatch[1].rm_so, NULL, 10);
let's use __virStrToLong_i internally
I think it makes that code a bit more readable too, +1
It is more readable because it doesn't give as detailed error reporting to the caller. I don't consider that a positive thing 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 :|
participants (4)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering