5:07 a.m.
"Daniel P. Berrange" <berrange(a)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;
}