[Libvir] PATCH: Fix block device numbers for virDomainBlockStats

[Jim Meyering]

"Daniel P. Berrange" <berrange at 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;
             }