[libvirt] [PATCH]: Rework xenLinuxDomainDeviceID to support more devices

[Jim Meyering]

Chris Lalancette <clalance at redhat.com> wrote:
...
> test suite.  As far as the error reporting goes, I won't argue that my patch
> gives slightly less information.  However, that being said, I have to believe
> that the most likely use of block statistics is something like:
>
> virsh dumpxml <dom>
> ...see what devices are listed there
> virsh domblkstats <dom> <device>
>
> In which case the slightly less verbose error reporting won't matter a whole lot.

Hi Chris,

...
> Index: src/stats_linux.c
> ===================================================================
...
> +static int
> +disk_re_match(const char *regex, const char *path, int *part)
> +{
> +    regex_t myreg;
> +    int err;
> +    int retval;
> +    regmatch_t pmatch[2];
> +
> +    retval = 0;
> +
> +    err = regcomp(&myreg, regex, REG_EXTENDED);
> +    if (err != 0)
> +        return 0;
> +
> +    err = regexec(&myreg, path, 2, pmatch, 0);
> +
> +    if (err == 0) {
> +        /* OK, we have a match; see if we have a partition */
> +        *part = 0;
> +        if (pmatch[1].rm_so != -1) {
> +            if (virStrToLong_i(path + pmatch[1].rm_so, NULL, 10, part) < 0)
> +                retval = 0;
> +            else
> +                retval = 1;
> +        }
> +        else
> +            retval = 1;
> +    }

How about dropping both "else" blocks and initializing "retval=1" above:

       if (err == 0) {
           /* OK, we have a match; see if we have a partition */
           *part = 0;
           retval = 1;
           if (pmatch[1].rm_so != -1) {
               if (virStrToLong_i(path + pmatch[1].rm_so, NULL, 10, part) < 0)
                   retval = 0;
           }
       }

You could even reduce the retval-setting part to a single statement:

    if (err == 0) {
        /* OK, we have a match; see if we have a partition */
        *part = 0;
        retval = (pmatch[1].rm_so == -1
                  || virStrToLong_i(path + pmatch[1].rm_so,
                                    NULL, 10, part) == 0);
    }

>  int
>  xenLinuxDomainDeviceID(virConnectPtr conn, int domid, const char *path)
>  {
> -    int disk, part = 0;
> -
> -    /* Strip leading path if any */
> -    if (strlen(path) > 5 &&
> -        STRPREFIX(path, "/dev/"))
> -        path += 5;
> +    int major, minor;
> +    int part;
> +    int retval;
> +    char *mod_path;
> +
> +    int scsi_majors[] = { SCSI_DISK0_MAJOR, SCSI_DISK1_MAJOR,

I know you just moved these, but they can be const:

       int const scsi_majors[] = {...

...
> +    int ide_majors[] = { IDE0_MAJOR, IDE1_MAJOR, IDE2_MAJOR, IDE3_MAJOR,

       int const ide_majors[] = { ...

...
> +    if (strlen(path) > 5 && STRPREFIX(path, "/dev/"))

Maybe use ">= 5", so that this code will preserve /dev/,
rather than turn it into "/dev//dev/" ?

> +        retval = asprintf(&mod_path, "%s", path);
> +    else
> +        retval = asprintf(&mod_path, "/dev/%s", path);
...
> +    if (disk_re_match("/dev/sd[a-z]([1-9]|1[0-5])?$", mod_path, &part)) {
> +        major = scsi_majors[(mod_path[7] - 'a') / 16];
> +        minor = ((mod_path[7] - 'a') % 16) * 16 + part;
> +        retval = major * 256 + minor;
> +    }
> +    else if (disk_re_match("/dev/sd[a-h][a-z]([1-9]|1[0-5])?$",
> +                           mod_path, &part) ||
> +             disk_re_match("/dev/sdi[a-v]([1-9]|1[0-5])?$",
> +                           mod_path, &part)) {
> +        major = scsi_majors[((mod_path[7] - 'a' + 1) * 26 + (mod_path[8] - 'a')) / 16];
> +        minor = (((mod_path[7] - 'a' + 1) * 26 + (mod_path[8] - 'a')) % 16)
> +            * 16 + part;
> +        retval = major * 256 + minor;
> +    }
> +    else if (disk_re_match("/dev/hd[a-t]([1-9]|[1-5][0-9]|6[0-3])?$",
> +                           mod_path, &part)) {
> +        major = ide_majors[(mod_path[7] - 'a') / 2];
> +        minor = ((mod_path[7] - 'a') % 2) * 64 + part;
> +        retval = major * 256 + minor;
>      }
> +    else if (disk_re_match("/dev/xvd[a-p]([1-9]|1[0-5])?$", mod_path, &part))
> +        retval = (202 << 8) + ((mod_path[8] - 'a') << 4) + part;
> +    else if (disk_re_match("/dev/xvd[q-z]([1-9]|1[0-5])?$", mod_path, &part))
> +        retval = (1 << 28) + ((mod_path[8] - 'a') << 8) + part;
> +    else if (disk_re_match("/dev/xvd[a-i][a-z]([1-9]|1[0-5])?$",
> +                           mod_path, &part))
> +        retval = (1 << 28) + (((mod_path[8] - 'a' + 1) * 26 + (mod_path[9] - 'a')) << 8) + part;

To retain the diagnostic Dan mentioned, you should be able to
insert something like this just before the final "else":

    else if (disk_re_match("/dev/sd[a-z]([0-9])+$", mod_path, &part)) {

> +    else
> +        statsErrorFunc (conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
> +                        "unsupported path, use xvdN, hdN, or sdN", domid);