On 12/01/2014 01:24 PM, Eduardo Costa wrote: Thanks for forwarding to the list. I'll modify the commit message to include a reference to https://bugzilla.redhat.com/show_bug.cgi?id=1169055 where you first posted this. > There is a race condition between the fopen and fscanf calls > in qemuGetProcessInfo. If fopen succeeds, there is a small > possibility that the file no longer exists before reading from it. > Now, if either fopen or fscanf calls fail, the function will behave > just as only fopen had failed. > --- > src/qemu/qemu_driver.c | 43 ++++++++++++++++--------------------------- > 1 file changed, 16 insertions(+), 27 deletions(-) > > > /* See 'man proc' for information about what all these fields are. We're > * only interested in a very few of them */ > - if (fscanf(pidinfo, > - /* pid -> stime */ Rather than reindenting all this,... > - "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu" > - /* cutime -> endcode */ > - "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u" > - /* startstack -> processor */ > - "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d", > - &usertime, &systime, &rss, &cpu) != 4) { > + if (pidinfo) { > + if (fscanf(pidinfo, ...why not just write: if (!pidinfo || fscanf(...) > + /* pid -> stime */ > + "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu" > + /* cutime -> endcode */ > + "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u" > + /* startstack -> processor */ > + "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d", > + &usertime, &systime, &rss, &cpu) != 4) { > + VIR_WARN("cannot parse process status data"); > + usertime = systime = 0; rss = 0; cpu = 0; Why explicitly set things back to 0? You already initialized them to sane values, and I'd rather use partial population than nothing at all (in all likelihood, since fscanf is buffered, you either got all-or-none on a single underlying read() from the file descriptor, depending on whether the race caused the read to fail because the process has died in the meantime). > + } > VIR_FORCE_FCLOSE(pidinfo); > - VIR_WARN("cannot parse process status data"); > - errno = -EINVAL; > - return -1; > } But this part I can agree with. > > /* We got jiffies > @@ -1374,8 +1365,6 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, > VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld", > (int) pid, tid, usertime, systime, cpu, rss); > > - VIR_FORCE_FCLOSE(pidinfo); > - If you simplify the above 'if' to avoid reindentation, then this is still necessary. Here's the simplified patch I'm pushing in your name; congratulations on your first libvirt patch! (Oh, and I reformatted a long line in the process of touching this function...) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a956c59..ec93191 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1310,9 +1310,9 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, { char *proc; FILE *pidinfo; - unsigned long long usertime, systime; - long rss; - int cpu; + unsigned long long usertime = 0, systime = 0; + long rss = 0; + int cpu = 0; int ret; /* In general, we cannot assume pid_t fits in int; but /proc parsing @@ -1324,22 +1324,13 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, if (ret < 0) return -1; - if (!(pidinfo = fopen(proc, "r"))) { - /* VM probably shut down, so fake 0 */ - if (cpuTime) - *cpuTime = 0; - if (lastCpu) - *lastCpu = 0; - if (vm_rss) - *vm_rss = 0; - VIR_FREE(proc); - return 0; - } + pidinfo = fopen(proc, "r"); VIR_FREE(proc); /* See 'man proc' for information about what all these fields are. We're * only interested in a very few of them */ - if (fscanf(pidinfo, + if (!pidinfo || + fscanf(pidinfo, /* pid -> stime */ "%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu" /* cutime -> endcode */ @@ -1347,10 +1338,7 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, /* startstack -> processor */ "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d", &usertime, &systime, &rss, &cpu) != 4) { - VIR_FORCE_FCLOSE(pidinfo); VIR_WARN("cannot parse process status data"); - errno = -EINVAL; - return -1; } /* We got jiffies @@ -1359,7 +1347,8 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, * So calculate thus.... */ if (cpuTime) - *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime) / (unsigned long long)sysconf(_SC_CLK_TCK); + *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime) + / (unsigned long long)sysconf(_SC_CLK_TCK); if (lastCpu) *lastCpu = cpu; -- 1.9.3 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list