"Daniel P. Berrange" <berrange@redhat.com> wrote:
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 ...
Nice patch! I spotted a few nits. You'll probably want to add statstest to the "TESTS" variable in tests/Makefile.am -- otherwise, "make check" won't run it. ...
+ if (path[4] != '\0') { + if (xstrtol_i(path+4, NULL, 10, &part) < 0 || + part < 1 || part > 15) {
Since the strto* functions (and hence the xstr* wrappers) accept leading white space and/or a leading '+', if you want to reject as "invalid ..." names like "xvd+2", "xvd 2" and maybe even "xvd02" and "xvd00000000000002", you can adjust that test e.g., if (!isdigit(path[4]) || path[4] == '0' || xstrtol_i(path+4, NULL, 10, &part) < 0 || part < 1 || part > 15) { ... Here's a minor problem I nearly missed because the string in question was wrapped off the right side of my ediff window (cough, 80-columns, cough ;-)
+ } 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);
That "sdp" in the diagnostic should be "sdt".
+ + 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",
Similarly with this "... 1 - 15" beyond the 80-col mark, I nearly failed to see the mismatch with the "part > 63" test. The diagnostic should say 1 - 63. Also, there was a little duplication in the sdX- and sdXX- partition-number handling code. Here's a patch that removes that and fixes the two typos: [and if you want to tighten up the xstr*-based parsing, this means there's one fewer use to change] --- src/stats_linux.c.~1~ 2008-01-28 11:16:14.000000000 +0100 +++ src/stats_linux.c 2008-01-28 11:25:19.000000000 +0100 @@ -289,6 +289,7 @@ 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) { @@ -298,23 +299,16 @@ 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; - } - } + p = path + 4; } 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; - } + p = path + 3; + } + if (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; } } @@ -328,7 +322,7 @@ 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", + "invalid path, device names must be in range sda - sdt", domid); return -1; } @@ -337,7 +331,7 @@ 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", + "invalid path, partition numbers for sdN must be in range 1 - 63", domid); return -1; }