"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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; } -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list