8:08 a.m.
Chris Lalancette <clalance(a)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);