Fix device name -> number conversion for block stats

ChangeLog

+Tue Jan 29 13:33:25 EST 2008  Daniel P. Berrange <berrange@redhat.com>
+
+	* src/stats_linux.c, src/stats_linux.h: Fix conversion of device
+	names into device numbers
+	* tests/.cvsignore, tests/Makefile.am, tests/statstest.c: Add
+	test case to validate device name -> number conversion
+
 Tue Jan 29 18:39:25 CET 2008  Jim Meyering  <meyering@redhat.com>
 
 	Also detect and remove unnecessary if-before-xmlXPathFreeContext.

src/stats_linux.c

@@ -18,6 +18,7 @@
 #include <fcntl.h>
 #include <string.h>
 #include <unistd.h>
+#include <ctype.h>
 
 #ifdef WITH_XEN
 #include <xs.h>
@@ -179,7 +180,7 @@ read_bd_stats (virConnectPtr conn, xenUnifiedPrivatePtr priv,
     if (stats->rd_req == -1 && stats->rd_bytes == -1 &&
         stats->wr_req == -1 && stats->wr_bytes == -1 &&
         stats->errs == -1) {
-        statsErrorFunc (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__,
+        statsErrorFunc (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
                         "Failed to read any block statistics", domid);
         return -1;
     }
@@ -192,7 +193,7 @@ read_bd_stats (virConnectPtr conn, xenUnifiedPrivatePtr priv,
         stats->wr_req == 0 && stats->wr_bytes == 0 &&
         stats->errs == 0 &&
         !check_bd_connected (priv, device, domid)) {
-        statsErrorFunc (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__,
+        statsErrorFunc (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
                         "Frontend block device not connected", domid);
         return -1;
     }
@@ -202,7 +203,7 @@ read_bd_stats (virConnectPtr conn, xenUnifiedPrivatePtr priv,
      */
     if (stats->rd_bytes > 0) {
         if (stats->rd_bytes >= ((unsigned long long)1)<<(63-9)) {
-            statsErrorFunc (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__,
+            statsErrorFunc (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
                             "stats->rd_bytes would overflow 64 bit counter",
                             domid);
             return -1;
@@ -211,7 +212,7 @@ read_bd_stats (virConnectPtr conn, xenUnifiedPrivatePtr priv,
     }
     if (stats->wr_bytes > 0) {
         if (stats->wr_bytes >= ((unsigned long long)1)<<(63-9)) {
-            statsErrorFunc (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__,
+            statsErrorFunc (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
                             "stats->wr_bytes would overflow 64 bit counter",
                             domid);
             return -1;
@@ -223,61 +224,147 @@ read_bd_stats (virConnectPtr conn, xenUnifiedPrivatePtr priv,
 }
 
 int
-xenLinuxDomainBlockStats (xenUnifiedPrivatePtr priv,
-                          virDomainPtr dom,
-                          const char *path,
-                          struct _virDomainBlockStats *stats)
+xenLinuxDomainDeviceID(virConnectPtr conn, int domid, const char *path)
 {
-    int minor, device;
-
-    /* Paravirt domains:
-     * Paths have the form "xvd[a-]" and map to paths
-     * /sys/devices/xen-backend/(vbd|tap)-domid-major:minor/
-     * statistics/(rd|wr|oo)_req.
-     * The major:minor is in this case fixed as 202*256 + minor*16
-     * where minor is 0 for xvda, 1 for xvdb and so on.
+    int disk, part = 0;
+
+    /* Strip leading path if any */
+    if (strlen(path) > 5 &&
+        STREQLEN(path, "/dev/", 5))
+        path += 5;
+
+    /*
+     * 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....
+     *
+     * The path for statistics will be
      *
-     * XXX Not clear what happens to device numbers for devices
-     * >= xdvo (minor >= 16), which would otherwise overflow the
-     * 256 minor numbers assigned to this major number.  So we
-     * currently limit you to the first 16 block devices per domain.
+     * /sys/devices/xen-backend/(vbd|tap)-{domid}-{devid}/statistics/{...}
      */
-    if (strlen (path) == 4 &&
+
+    if (strlen (path) >= 4 &&
         STREQLEN (path, "xvd", 3)) {
-        if ((minor = path[3] - 'a') < 0 || minor >= 16) {
-            statsErrorFunc (dom->conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
-                            "invalid path, should be xvda, xvdb, etc.",
-                            dom->id);
+        /* 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;
         }
-        device = XENVBD_MAJOR * 256 + minor * 16;
 
-        return read_bd_stats (dom->conn, priv, device, dom->id, stats);
-    }
-    /* Fullvirt domains:
-     * hda, hdb etc map to major = HD_MAJOR*256 + minor*16.
-     *
-     * See comment above about devices >= hdo.
-     */
-    else if (strlen (path) == 3 &&
-             STREQLEN (path, "hd", 2)) {
-        if ((minor = path[2] - 'a') < 0 || minor >= 16) {
-            statsErrorFunc (dom->conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
-                            "invalid path, should be hda, hdb, etc.",
-                            dom->id);
+        if (path[4] != '\0') {
+            if (!isdigit(path[4]) || path[4] == '0' ||
+                xstrtol_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 &&
+               STREQLEN (path, "sd", 2)) {
+        /* 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;
         }
-        device = HD_MAJOR * 256 + minor * 16;
+        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 && (!isdigit(*p) || *p == '0' ||
+                      xstrtol_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 read_bd_stats (dom->conn, priv, device, dom->id, stats);
+        return (majors[disk/16] * 256) + ((disk%16) * 16) + part;
+    } else if (strlen (path) >= 3 &&
+               STREQLEN (path, "hd", 2)) {
+        /* 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 (path[3] != '\0') {
+            if (!isdigit(path[3]) || path[3] == '0' ||
+                xstrtol_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;
+            }
+        }
+
+        return (majors[disk/2] * 256) + ((disk % 2) * 63) + part;
     }
 
     /* Otherwise, unsupported device name. */
-    statsErrorFunc (dom->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__,
-                    "unsupported path (use xvda, hda, etc.)", dom->id);
+    statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
+                    "unsupported path, use xvdN, hdN, or sdN", domid);
     return -1;
 }
 
+int
+xenLinuxDomainBlockStats (xenUnifiedPrivatePtr priv,
+                          virDomainPtr dom,
+                          const char *path,
+                          struct _virDomainBlockStats *stats)
+{
+    int device = xenLinuxDomainDeviceID(dom->conn, dom->id, path);
+
+    if (device < 0)
+        return -1;
+
+    return read_bd_stats (dom->conn, priv, device, dom->id, stats);
+}
+
 #endif /* WITH_XEN */
 
 /*-------------------- interface stats --------------------*/
@@ -296,7 +383,7 @@ linuxDomainInterfaceStats (virConnectPtr conn, const char *path,
 
     fp = fopen ("/proc/net/dev", "r");
     if (!fp) {
-        statsErrorFunc (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__,
+        statsErrorFunc (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
                         "/proc/net/dev", errno);
         return -1;
     }
@@ -352,7 +439,7 @@ linuxDomainInterfaceStats (virConnectPtr conn, const char *path,
     }
     fclose (fp);
 
-    statsErrorFunc (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__,
+    statsErrorFunc (conn, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
                     "/proc/net/dev: Interface not found", 0);
     return -1;
 }

src/stats_linux.h

@@ -21,6 +21,8 @@ extern int xenLinuxDomainBlockStats (xenUnifiedPrivatePtr priv,
 extern int linuxDomainInterfaceStats (virConnectPtr conn, const char *path,
 				      struct _virDomainInterfaceStats *stats);
 
+extern int xenLinuxDomainDeviceID(virConnectPtr conn, int domid, const char *dev);
+
 #endif /* __linux__ */
 
 #endif /* __STATS_LINUX_H__ */

tests/.cvsignore

@@ -13,6 +13,7 @@ xencapstest
 qemuxml2xmltest
 qemuxml2argvtest
 nodeinfotest
+statstest
 *.gcda
 *.gcno
 

tests/Makefile.am

@@ -44,7 +44,7 @@ EXTRA_DIST =		\
 
 noinst_PROGRAMS = xmlrpctest xml2sexprtest sexpr2xmltest virshtest conftest \
 	reconnect xmconfigtest xencapstest qemuxml2argvtest qemuxml2xmltest \
-        nodeinfotest
+        nodeinfotest statstest
 
 test_scripts = \
 	daemon-conf \
@@ -54,7 +54,7 @@ EXTRA_DIST += $(test_scripts)
 
 TESTS = xml2sexprtest sexpr2xmltest virshtest test_conf.sh xmconfigtest \
         xencapstest qemuxml2argvtest qemuxml2xmltest nodeinfotest \
-	$(test_scripts)
+	statstest $(test_scripts)
 if ENABLE_XEN_TESTS
   TESTS += reconnect
 endif
@@ -122,6 +122,10 @@ nodeinfotest_SOURCES = \
 	nodeinfotest.c testutils.h testutils.c
 nodeinfotest_LDADD = $(LDADDS)
 
+statstest_SOURCES = \
+	statstest.c testutils.h testutils.c
+statstest_LDADD = $(LDADDS)
+
 reconnect_SOURCES = \
 	reconnect.c
 reconnect_LDADD = $(LDADDS)

tests/statstest.c

+#include "config.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "stats_linux.h"
+#include "internal.h"
+
+static void testQuietError(void *userData ATTRIBUTE_UNUSED, virErrorPtr error ATTRIBUTE_UNUSED)
+{
+    /* nada */
+}
+
+#ifdef __linux__
+static int testDevice(const char *path, int expect)
+{
+    int actual = xenLinuxDomainDeviceID(NULL, 1, path);
+
+    if (actual == expect) {
+        fprintf(stderr, "%-14s == %-6d           OK\n", path, expect);
+        return 0;
+    } else {
+        fprintf(stderr, "%-14s == %-6d (%-6d)  FAILED\n", path, expect, actual);
+        return -1;
+    }
+}
+#endif
+
+int
+main(void)
+{
+    int ret = 0;
+#ifdef __linux__
+    /* Some of our tests delibrately test failure cases, so
+     * register a handler to stop error messages cluttering
+     * up display
+     */
+    if (!getenv("DEBUG_TESTS"))
+        virSetErrorFunc(NULL, testQuietError);
+
+    /********************************
+     * Xen paravirt disks
+     ********************************/
+
+    /* first valid disk */
+    if (testDevice("xvda", 51712) < 0)
+        ret = -1;
+    if (testDevice("xvda1", 51713) < 0)
+        ret = -1;
+    if (testDevice("xvda15", 51727) < 0)
+        ret = -1;
+    /* Last valid disk */
+    if (testDevice("xvdp", 51952) < 0)
+        ret = -1;
+    if (testDevice("xvdp1", 51953) < 0)
+        ret = -1;
+    if (testDevice("xvdp15", 51967) < 0)
+        ret = -1;
+
+    /* Disk letter to large */
+    if (testDevice("xvdq", -1) < 0)
+        ret = -1;
+    /* missing disk letter */
+    if (testDevice("xvd1", -1) < 0)
+        ret = -1;
+    /* partition to large */
+    if (testDevice("xvda16", -1) < 0)
+        ret = -1;
+    /* partition to small */
+    if (testDevice("xvda0", -1) < 0)
+        ret = -1;
+    /* leading zeros */
+    if (testDevice("xvda01", -1) < 0)
+        ret = -1;
+    /* leading + */
+    if (testDevice("xvda+1", -1) < 0)
+        ret = -1;
+    /* leading - */
+    if (testDevice("xvda-1", -1) < 0)
+        ret = -1;
+
+    /********************************
+     * IDE disks
+     ********************************/
+
+    /* odd numbered disk */
+    if (testDevice("hda", 768) < 0)
+        ret = -1;
+    if (testDevice("hda1", 769) < 0)
+        ret = -1;
+    if (testDevice("hda63", 831) < 0)
+        ret = -1;
+    /* even number disk */
+    if (testDevice("hdd", 5695) < 0)
+        ret = -1;
+    if (testDevice("hdd1", 5696) < 0)
+        ret = -1;
+    if (testDevice("hdd63", 5758) < 0)
+        ret = -1;
+    /* last valid disk */
+    if (testDevice("hdt", 23359) < 0)
+        ret = -1;
+    if (testDevice("hdt1", 23360) < 0)
+        ret = -1;
+    if (testDevice("hdt63", 23422) < 0)
+        ret = -1;
+
+    /* Disk letter to large */
+    if (testDevice("hdu", -1) < 0)
+        ret = -1;
+    /* missing disk letter */
+    if (testDevice("hd1", -1) < 0)
+        ret = -1;
+    /* partition to large */
+    if (testDevice("hda64", -1) < 0)
+        ret = -1;
+    /* partition to small */
+    if (testDevice("hda0", -1) < 0)
+        ret = -1;
+
+
+
+    /********************************
+     * SCSI disks
+     ********************************/
+
+    /* first valid disk */
+    if (testDevice("sda", 2048) < 0)
+        ret = -1;
+    if (testDevice("sda1", 2049) < 0)
+        ret = -1;
+    if (testDevice("sda15", 2063) < 0)
+        ret = -1;
+    /* last valid disk of first SCSI major number */
+    if (testDevice("sdp", 2288) < 0)
+        ret = -1;
+    if (testDevice("sdp1", 2289) < 0)
+        ret = -1;
+    if (testDevice("sdp15", 2303) < 0)
+        ret = -1;
+    /* first valid disk of second SCSI major number */
+    if (testDevice("sdq", 16640) < 0)
+        ret = -1;
+    if (testDevice("sdq1", 16641) < 0)
+        ret = -1;
+    if (testDevice("sdq15", 16655) < 0)
+        ret = -1;
+    /* last valid single letter disk */
+    if (testDevice("sdz", 16784) < 0)
+        ret = -1;
+    if (testDevice("sdz1", 16785) < 0)
+        ret = -1;
+    if (testDevice("sdz15", 16799) < 0)
+        ret = -1;
+    /* first valid dual letter disk */
+    if (testDevice("sdaa", 16800) < 0)
+        ret = -1;
+    if (testDevice("sdaa1", 16801) < 0)
+        ret = -1;
+    if (testDevice("sdaa15", 16815) < 0)
+        ret = -1;
+    /* second valid dual letter disk */
+    if (testDevice("sdab", 16816) < 0)
+        ret = -1;
+    if (testDevice("sdab1", 16817) < 0)
+        ret = -1;
+    if (testDevice("sdab15", 16831) < 0)
+        ret = -1;
+    /* first letter of second sequence of dual letter disk */
+    if (testDevice("sdba", 17216) < 0)
+        ret = -1;
+    if (testDevice("sdba1", 17217) < 0)
+        ret = -1;
+    if (testDevice("sdba15", 17231) < 0)
+        ret = -1;
+    /* last valid dual letter disk */
+    if (testDevice("sdiv", 34800) < 0)
+        ret = -1;
+    if (testDevice("sdiv1", 34801) < 0)
+        ret = -1;
+    if (testDevice("sdiv15", 34815) < 0)
+        ret = -1;
+
+    /* Disk letter to large */
+    if (testDevice("sdix", -1) < 0)
+        ret = -1;
+    /* missing disk letter */
+    if (testDevice("sd1", -1) < 0)
+        ret = -1;
+    /* partition to large */
+    if (testDevice("sda16", -1) < 0)
+        ret = -1;
+    /* partition to small */
+    if (testDevice("sda0", -1) < 0)
+        ret = -1;
+
+
+    /* Path stripping */
+    if (testDevice("/dev", -1) < 0)
+        ret = -1;
+    if (testDevice("/dev/", -1) < 0)
+        ret = -1;
+    if (testDevice("/dev/xvd", -1) < 0)
+        ret = -1;
+    if (testDevice("/dev/xvda", 51712) < 0)
+        ret = -1;
+    if (testDevice("/dev/xvda1", 51713) < 0)
+        ret = -1;
+    if (testDevice("/dev/xvda15", 51727) < 0)
+        ret = -1;
+
+#endif
+    exit(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
+}
+
+/*
+ * Local variables:
+ *  indent-tabs-mode: nil
+ *  c-indent-level: 4
+ *  c-basic-offset: 4
+ *  tab-width: 4
+ * End:
+ */