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
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 13:42:11 -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,134 @@
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[3];
+
+ retval = 0;
+
+ err = regcomp(&myreg, regex, REG_EXTENDED);
+ if (err != 0)
+ return 0;
+
+ err = regexec(&myreg, path, 3, pmatch, 0);
+
+ 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;
+ }
+ }
+
+ 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 const 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 const 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;
+ /*
+ * OK, we've now checked the common case (things that work); check the
+ * beginning of the strings for better error messages
+ */
+ else if (strlen(mod_path) >= 7 && STRPREFIX(mod_path, "/dev/sd"))
+ statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
+ "invalid path, device names must be in the range sda[1-15] - sdiv[1-15]",
+ domid);
+ else if (strlen(mod_path) >= 7 && STRPREFIX(mod_path, "/dev/hd"))
+ statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
+ "invalid path, device names must be in the range hda[1-63] - hdt[1-63]",
+ domid);
+ else if (strlen(mod_path) >= 8 && STRPREFIX(mod_path, "/dev/xvd"))
+ statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
+ "invalid path, device names must be in the range xvda[1-15] - xvdiz[1-15]",
+ domid);
+ 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
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 13:42:11 -0000
@@ -70,22 +70,34 @@
* Xen paravirt disks
********************************/
+ DO_TEST("xvd", -1);
+
/* first valid disk */
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 +110,36 @@
* IDE disks
********************************/
- /* odd numbered disk */
+ DO_TEST("hd", -1);
+
+ /* 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 numbered 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);
@@ -126,6 +148,8 @@
* SCSI disks
********************************/
+ DO_TEST("sd", -1);
+
/* first valid disk */
DO_TEST("sda", 2048);
DO_TEST("sda1", 2049);
@@ -159,13 +183,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);
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list