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
Index: tests/statstest.c
===================================================================
RCS file: /data/cvs/libvirt/tests/statstest.c,v
retrieving revision 1.7
diff -u -r1.7 statstest.c
--- a/tests/statstest.c 29 May 2008 15:31:49 -0000 1.7
+++ b/tests/statstest.c 5 Aug 2008 12:08:27 -0000
@@ -74,18 +74,28 @@
DO_TEST("xvda", 51712);
DO_TEST("xvda1", 51713);
DO_TEST("xvda15", 51727);
- /* Last valid disk */
+ /* Last non-extended disk */
DO_TEST("xvdp", 51952);
DO_TEST("xvdp1", 51953);
DO_TEST("xvdp15", 51967);
- /* Disk letter to large */
- DO_TEST("xvdq", -1);
+ /* First extended disk */
+ DO_TEST("xvdq", 268439552);
+ DO_TEST("xvdq1", 268439553);
+ DO_TEST("xvdq15", 268439567);
+ /* Last extended disk */
+ DO_TEST("xvdiz", 268501760);
+ DO_TEST("xvdiz1", 268501761);
+ DO_TEST("xvdiz15", 268501775);
+
+ /* Disk letter too large */
+ DO_TEST("xvdja", -1);
+
/* missing disk letter */
DO_TEST("xvd1", -1);
- /* partition to large */
+ /* partition too large */
DO_TEST("xvda16", -1);
- /* partition to small */
+ /* partition too small */
DO_TEST("xvda0", -1);
/* leading zeros */
DO_TEST("xvda01", -1);
@@ -98,26 +108,34 @@
* IDE disks
********************************/
- /* odd numbered disk */
+ /* first numbered disk */
DO_TEST("hda", 768);
DO_TEST("hda1", 769);
DO_TEST("hda63", 831);
- /* even number disk */
- DO_TEST("hdd", 5695);
- DO_TEST("hdd1", 5696);
- DO_TEST("hdd63", 5758);
+ /* second numbered disk */
+ DO_TEST("hdb", 832);
+ DO_TEST("hdb1", 833);
+ DO_TEST("hdb63", 895);
+ /* third numbered disk */
+ DO_TEST("hdc", 5632);
+ DO_TEST("hdc1", 5633);
+ DO_TEST("hdc63", 5695);
+ /* fourth number disk */
+ DO_TEST("hdd", 5696);
+ DO_TEST("hdd1", 5697);
+ DO_TEST("hdd63", 5759);
/* last valid disk */
- DO_TEST("hdt", 23359);
- DO_TEST("hdt1", 23360);
- DO_TEST("hdt63", 23422);
+ DO_TEST("hdt", 23360);
+ DO_TEST("hdt1", 23361);
+ DO_TEST("hdt63", 23423);
/* Disk letter to large */
DO_TEST("hdu", -1);
/* missing disk letter */
DO_TEST("hd1", -1);
- /* partition to large */
+ /* partition too large */
DO_TEST("hda64", -1);
- /* partition to small */
+ /* partition too small */
DO_TEST("hda0", -1);
@@ -159,13 +177,13 @@
DO_TEST("sdiv1", 34801);
DO_TEST("sdiv15", 34815);
- /* Disk letter to large */
+ /* Disk letter too large */
DO_TEST("sdix", -1);
/* missing disk letter */
DO_TEST("sd1", -1);
- /* partition to large */
+ /* partition too large */
DO_TEST("sda16", -1);
- /* partition to small */
+ /* partition too small */
DO_TEST("sda0", -1);
Index: src/stats_linux.c
===================================================================
RCS file: /data/cvs/libvirt/src/stats_linux.c,v
retrieving revision 1.15
diff -u -r1.15 stats_linux.c
--- a/src/stats_linux.c 23 May 2008 08:24:44 -0000 1.15
+++ b/src/stats_linux.c 5 Aug 2008 12:08:27 -0000
@@ -18,6 +18,7 @@
#include <fcntl.h>
#include <string.h>
#include <unistd.h>
+#include <regex.h>
#include "c-ctype.h"
#ifdef WITH_XEN
@@ -28,6 +29,7 @@
#include "util.h"
#include "xen_unified.h"
#include "stats_linux.h"
+#include "memory.h"
/**
* statsErrorFunc:
@@ -224,132 +226,121 @@
return 0;
}
+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;
+ }
+
+ regfree(&myreg);
+
+ return retval;
+}
+
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,
+ SCSI_DISK2_MAJOR, SCSI_DISK3_MAJOR,
+ SCSI_DISK4_MAJOR, SCSI_DISK5_MAJOR,
+ SCSI_DISK6_MAJOR, SCSI_DISK7_MAJOR,
+ SCSI_DISK8_MAJOR, SCSI_DISK9_MAJOR,
+ SCSI_DISK10_MAJOR, SCSI_DISK11_MAJOR,
+ SCSI_DISK12_MAJOR, SCSI_DISK13_MAJOR,
+ SCSI_DISK14_MAJOR, SCSI_DISK15_MAJOR };
+ int ide_majors[] = { IDE0_MAJOR, IDE1_MAJOR, IDE2_MAJOR, IDE3_MAJOR,
+ IDE4_MAJOR, IDE5_MAJOR, IDE6_MAJOR, IDE7_MAJOR,
+ IDE8_MAJOR, IDE9_MAJOR };
/*
* Possible block device majors & partition ranges. This
* matches the ranges supported in Xend xen/util/blkif.py
*
- * hdNM: N=a-t, M=1-63, major={IDE0_MAJOR -> IDE9_MAJOR}
- * sdNM: N=a-z,aa-iv, M=1-15, major={SCSI_DISK0_MAJOR -> SCSI_DISK15_MAJOR}
- * xvdNM: N=a-p, M=1-15, major=XENVBD_MAJOR
- *
- * NB, the SCSI major isn't technically correct, as XenD only knows
- * about major=8. We cope with all SCSI majors in anticipation of
- * XenD perhaps being fixed one day....
+ * hdNM: N=a-t, M=1-63, major={IDE0_MAJOR -> IDE9_MAJOR}
+ * sdNM: N=a-z,aa-iv, M=1-15, major={SCSI_DISK0_MAJOR -> SCSI_DISK15_MAJOR}
+ * xvdNM: N=a-p M=1-15, major=XENVBD_MAJOR
+ * xvdNM: N=q-z,aa-iz M=1-15, major=(1<<28)
*
* The path for statistics will be
*
* /sys/devices/xen-backend/(vbd|tap)-{domid}-{devid}/statistics/{...}
*/
- 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;
- }
-
- if (path[4] != '\0') {
- if (!c_isdigit(path[4]) || path[4] == '0' ||
- virStrToLong_i(path+4, NULL, 10, &part) < 0 ||
- part < 1 || part > 15) {
- statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
- "invalid path, partition numbers for xvdN must be in range 1 - 15",
- domid);
- return -1;
- }
- }
-
- return (XENVBD_MAJOR * 256) + (disk * 16) + part;
- } else if (strlen (path) >= 3 &&
- STRPREFIX (path, "sd")) {
- /* SCSI device handling */
- int majors[] = { SCSI_DISK0_MAJOR, SCSI_DISK1_MAJOR, SCSI_DISK2_MAJOR,
- SCSI_DISK3_MAJOR, SCSI_DISK4_MAJOR, SCSI_DISK5_MAJOR,
- SCSI_DISK6_MAJOR, SCSI_DISK7_MAJOR, SCSI_DISK8_MAJOR,
- SCSI_DISK9_MAJOR, SCSI_DISK10_MAJOR, SCSI_DISK11_MAJOR,
- SCSI_DISK12_MAJOR, SCSI_DISK13_MAJOR, SCSI_DISK14_MAJOR,
- SCSI_DISK15_MAJOR };
-
- disk = (path[2] - 'a');
- if (disk < 0 || disk > 25) {
- statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
- "invalid path, device names must be in range sda - sdiv",
- domid);
- return -1;
- }
- if (path[3] != '\0') {
- const char *p = NULL;
- if (path[3] >= 'a' && path[3] <= 'z') {
- disk = ((disk + 1) * 26) + (path[3] - 'a');
- if (disk < 0 || disk > 255) {
- statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
- "invalid path, device names must be in range sda - sdiv",
- domid);
- return -1;
- }
-
- if (path[4] != '\0')
- p = path + 4;
- } else {
- p = path + 3;
- }
- if (p && (!c_isdigit(*p) || *p == '0' ||
- virStrToLong_i(p, NULL, 10, &part) < 0 ||
- part < 1 || part > 15)) {
- statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
- "invalid path, partition numbers for sdN must be in range 1 - 15",
- domid);
- return -1;
- }
- }
-
- return (majors[disk/16] * 256) + ((disk%16) * 16) + part;
- } else if (strlen (path) >= 3 &&
- STRPREFIX (path, "hd")) {
- /* IDE device handling */
- int majors[] = { IDE0_MAJOR, IDE1_MAJOR, IDE2_MAJOR, IDE3_MAJOR,
- IDE4_MAJOR, IDE5_MAJOR, IDE6_MAJOR, IDE7_MAJOR,
- IDE8_MAJOR, IDE9_MAJOR };
- disk = (path[2] - 'a');
- if (disk < 0 || disk > 19) {
- statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
- "invalid path, device names must be in range hda - hdt",
- domid);
- return -1;
- }
+ if (strlen(path) > 5 && STRPREFIX(path, "/dev/"))
+ retval = asprintf(&mod_path, "%s", path);
+ else
+ retval = asprintf(&mod_path, "/dev/%s", path);
+
+ if (retval < 0) {
+ statsErrorFunc (conn, VIR_ERR_NO_MEMORY, __FUNCTION__,
+ "allocating mod_path", domid);
+ return -1;
+ }
- if (path[3] != '\0') {
- if (!c_isdigit(path[3]) || path[3] == '0' ||
- virStrToLong_i(path+3, NULL, 10, &part) < 0 ||
- part < 1 || part > 63) {
- statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
- "invalid path, partition numbers for hdN must be in range 1 - 63",
- domid);
- return -1;
- }
- }
+ retval = -1;
- 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-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;
+ else
+ statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
+ "unsupported path, use xvdN, hdN, or sdN", domid);
- /* Otherwise, unsupported device name. */
- statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
- "unsupported path, use xvdN, hdN, or sdN", domid);
- return -1;
+ VIR_FREE(mod_path);
+
+ return retval;
}
int
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list