Chris Lalancette <clalance@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);