[Libvir] PATCH: Fix block device numbers for virDomainBlockStats

As mentioned in this thread: http://www.redhat.com/archives/libvir-list/2008-January/msg00475.html The code for calculating block devices numbers has a number of flaws. This patch fixes the flaws and adds a comprehensive test program to validate it. The test program also validates invalid cases. NB, this code is actually now more perfect than Xen, since Xen doesn't handle SCSI disks > sdp, even though you can go all the way to sdhv. The rules are: * hdNM: N=a-t, M=1-63, major={IDE0_MAJOR -> IDE9_MAJOR} * sdNM: N=a-z,aa-hw, M=1-15, major={SCSI_DISK0_MAJOR -> SCSI_DISK15_MAJOR} * xvdNM: N=a-p, M=1-15, major=XENVBD_MAJOR Which translates to the formulas xvdNM: (XENVBD_MAJOR * 256) + (disk * 16) + part; sdNM: (majors[disk/16] * 256) + ((disk%16) * 16) + part hdNM: (majors[disk/2] * 256) + ((disk % 2) * 63) + part; The SCSI/IDE formulas lookup the major number in a table since they are allocated non-contigously. The test case validates these: $ ./statstest xvda == 51712 OK xvda1 == 51713 OK xvda15 == 51727 OK xvdp == 51952 OK xvdp1 == 51953 OK xvdp15 == 51967 OK xvdq == -1 OK xvd1 == -1 OK xvda16 == -1 OK xvda0 == -1 OK hda == 768 OK hda1 == 769 OK hda63 == 831 OK hdd == 5695 OK hdd1 == 5696 OK hdd63 == 5758 OK hdt == 23359 OK hdt1 == 23360 OK hdt63 == 23422 OK hdu == -1 OK hd1 == -1 OK hda64 == -1 OK hda0 == -1 OK sda == 2048 OK sda1 == 2049 OK sda15 == 2063 OK sdp == 2288 OK sdp1 == 2289 OK sdp15 == 2303 OK sdq == 16640 OK sdq1 == 16641 OK sdq15 == 16655 OK sdz == 16784 OK sdz1 == 16785 OK sdz15 == 16799 OK sdaa == 16800 OK sdaa1 == 16801 OK sdaa15 == 16815 OK sdab == 16816 OK sdab1 == 16817 OK sdab15 == 16831 OK sdba == 17216 OK sdba1 == 17217 OK sdba15 == 17231 OK sdiv == 34800 OK sdiv1 == 34801 OK sdiv15 == 34815 OK sdix == -1 OK sd1 == -1 OK sda16 == -1 OK sda0 == -1 OK /dev == -1 OK /dev/ == -1 OK /dev/xvd == -1 OK /dev/xvda == 51712 OK /dev/xvda1 == 51713 OK /dev/xvda15 == 51727 OK The patch also fixes a couple of error codes to be INTERNAL_ERROR rather than NO_SUPPORT, because the latter is for APIs which are not supported, where as the conditions here are simply errors that shouldn't happen. Dan. Index: src/stats_linux.c =================================================================== RCS file: /data/cvs/libvirt/src/stats_linux.c,v retrieving revision 1.6 diff -u -p -r1.6 stats_linux.c --- src/stats_linux.c 20 Nov 2007 10:58:21 -0000 1.6 +++ src/stats_linux.c 27 Jan 2008 20:08:27 -0000 @@ -179,7 +179,7 @@ read_bd_stats (virConnectPtr conn, xenUn 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 +192,7 @@ read_bd_stats (virConnectPtr conn, xenUn 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 +202,7 @@ read_bd_stats (virConnectPtr conn, xenUn */ 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 +211,7 @@ read_bd_stats (virConnectPtr conn, xenUn } 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 +223,149 @@ read_bd_stats (virConnectPtr conn, xenUn } int -xenLinuxDomainBlockStats (xenUnifiedPrivatePtr priv, - virDomainPtr dom, - const char *path, - struct _virDomainBlockStats *stats) +xenLinuxDomainDeviceID(virConnectPtr conn, int domid, const char *path) { - int minor, device; + int disk, part = 0; - /* 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. + /* 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-hw, M=1-15, major={SCSI_DISK0_MAJOR -> SCSI_DISK15_MAJOR} + * xvdNM: N=a-p, M=1-15, major=XENVBD_MAJOR * - * 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. + * NB, the SCSI major isn't technically correct, as there can be + * many SCSI major numbers as per IDE majors. XenD only knows about + * major=8 so we follow same limitation (for now). + * + * The path for statistics will be + * + * /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 (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 - sdiw", + domid); return -1; } - device = HD_MAJOR * 256 + minor * 16; + if (path[3] != '\0') { + 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 - sdiw", + domid); + return -1; + } + + if (path[4] != '\0') { + if (xstrtol_i(path+4, 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; + } + } + } else { + if (xstrtol_i(path+3, 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 sda - sdp", + domid); + return -1; + } + + if (path[3] != '\0') { + if (xstrtol_i(path+3, NULL, 10, &part) < 0 || + part < 1 || part > 63) { + 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/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 +384,7 @@ linuxDomainInterfaceStats (virConnectPtr 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 +440,7 @@ linuxDomainInterfaceStats (virConnectPtr } 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; } Index: src/stats_linux.h =================================================================== RCS file: /data/cvs/libvirt/src/stats_linux.h,v retrieving revision 1.1 diff -u -p -r1.1 stats_linux.h --- src/stats_linux.h 14 Nov 2007 11:58:36 -0000 1.1 +++ src/stats_linux.h 27 Jan 2008 20:08:27 -0000 @@ -21,6 +21,8 @@ extern int xenLinuxDomainBlockStats (xen 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__ */ Index: tests/.cvsignore =================================================================== RCS file: /data/cvs/libvirt/tests/.cvsignore,v retrieving revision 1.10 diff -u -p -r1.10 .cvsignore --- tests/.cvsignore 25 Jul 2007 23:16:30 -0000 1.10 +++ tests/.cvsignore 27 Jan 2008 20:08:27 -0000 @@ -13,6 +13,7 @@ xencapstest qemuxml2xmltest qemuxml2argvtest nodeinfotest +statstest *.gcda *.gcno Index: tests/Makefile.am =================================================================== RCS file: /data/cvs/libvirt/tests/Makefile.am,v retrieving revision 1.34 diff -u -p -r1.34 Makefile.am --- tests/Makefile.am 12 Dec 2007 07:21:37 -0000 1.34 +++ tests/Makefile.am 27 Jan 2008 20:08:27 -0000 @@ -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 \ @@ -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) Index: tests/statstest.c =================================================================== RCS file: tests/statstest.c diff -N tests/statstest.c --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/statstest.c 27 Jan 2008 20:08:27 -0000 @@ -0,0 +1,218 @@ +#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; + + + /******************************** + * 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; + /* first 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; + /* 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 */ + + /* first valid disk */ + 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: + */ Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|