On 05/23/2017 11:31 PM, Wang King wrote: > qemuGetProcessInfo is more likely a process utility function, just rename it > to virProcessGetStat and move it to virprocess.c source file. > --- > src/libvirt_private.syms | 1 + > src/qemu/qemu_driver.c | 83 ++++++++++-------------------------------------- > src/util/virprocess.c | 62 ++++++++++++++++++++++++++++++++++++ > src/util/virprocess.h | 4 +++ > 4 files changed, 83 insertions(+), 67 deletions(-) > You should follow the guidelines about a cover letter when sending more than one patch (http://libvirt.org/hacking.html). It would have especially helped explain why you're doing this... Beyond that there's quite a bit of linux specific stuff in both of these calls and moves from the "/proc" file system to the "sysconf" call in this patch and the comments in the second one related to CONFIG_SCHED*. In other virprocess.c functions you'll generally note there are two functions - one to handle linux and the other to return an error if run on a non linux system (forcing someone to add some code in order to make this work there). Not sure I see the need to move and rename these, but you can try again with a more complete set of patches. ... note one more comment below... > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index d361454..3681869 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2382,6 +2382,7 @@ virProcessGetMaxMemLock; > virProcessGetNamespaces; > virProcessGetPids; > virProcessGetStartTime; > +virProcessGetStat; > virProcessKill; > virProcessKillPainfully; > virProcessNamespaceAvailable; > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 6c79d4f..a4aa5da 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -1378,68 +1378,6 @@ qemuGetSchedInfo(unsigned long long *cpuWait, > return ret; > } > > - > -static int > -qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss, > - pid_t pid, int tid) > -{ > - char *proc; > - FILE *pidinfo; > - 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 > - * is specific to Linux where int works fine. */ > - if (tid) > - ret = virAsprintf(&proc, "/proc/%d/task/%d/stat", (int) pid, tid); > - else > - ret = virAsprintf(&proc, "/proc/%d/stat", (int) pid); > - if (ret < 0) > - return -1; > - > - 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 (!pidinfo || > - fscanf(pidinfo, > - /* pid -> stime */ > - "%*d (%*[^)]) %*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"); > - } > - > - /* We got jiffies > - * We want nanoseconds > - * _SC_CLK_TCK is jiffies per second > - * So calculate thus.... > - */ > - if (cpuTime) > - *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime) > - / (unsigned long long)sysconf(_SC_CLK_TCK); > - if (lastCpu) > - *lastCpu = cpu; > - > - if (vm_rss) > - *vm_rss = rss * virGetSystemPageSizeKB(); > - > - > - 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); > - > - return 0; > -} > - > - > static int > qemuDomainHelperGetVcpus(virDomainObjPtr vm, > virVcpuInfoPtr info, > @@ -1482,9 +1420,9 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, > vcpuinfo->number = i; > vcpuinfo->state = VIR_VCPU_RUNNING; > > - if (qemuGetProcessInfo(&vcpuinfo->cpuTime, > - &vcpuinfo->cpu, NULL, > - vm->pid, vcpupid) < 0) { > + if (virProcessGetStat(vm->pid, vcpupid, > + &vcpuinfo->cpuTime, > + &vcpuinfo->cpu, NULL) < 0) { > virReportSystemError(errno, "%s", > _("cannot get vCPU placement & pCPU time")); > return -1; > @@ -2641,7 +2579,7 @@ qemuDomainGetInfo(virDomainPtr dom, > } > > if (virDomainObjIsActive(vm)) { > - if (qemuGetProcessInfo(&(info->cpuTime), NULL, NULL, vm->pid, 0) < 0) { > + if (virProcessGetStat(vm->pid, 0, &(info->cpuTime), NULL, NULL) < 0) { > virReportError(VIR_ERR_OPERATION_FAILED, "%s", > _("cannot read cputime for domain")); > goto cleanup; > @@ -8172,6 +8110,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, > virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; > bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; > int ret = -1; > + virQEMUCapsPtr qemuCaps = NULL; > + qemuDomainObjPrivatePtr priv; > virQEMUDriverConfigPtr cfg = NULL; > virCapsPtr caps = NULL; > unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; > @@ -8190,6 +8130,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, > if (!(vm = qemuDomObjFromDomain(dom))) > goto cleanup; > > + priv = vm->privateData; > + > if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) > goto cleanup; > > @@ -8216,6 +8158,12 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, > goto endjob; > } > > + if (priv->qemuCaps) > + qemuCaps = virObjectRef(priv->qemuCaps); > + else if (!(qemuCaps = virQEMUCapsCacheLookup(caps, driver->qemuCapsCache, > + vm->def->emulator))) > + goto endjob; > + Is this a related hunk? or something else that should have been in it's own patch? John > if (flags & VIR_DOMAIN_AFFECT_CONFIG) { > /* Make a copy for updated domain. */ > vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); > @@ -8263,6 +8211,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, > qemuDomainObjEndJob(driver, vm); > > cleanup: > + virObjectUnref(qemuCaps); > virDomainDefFree(vmdef); > if (dev != dev_copy) > virDomainDeviceDefFree(dev_copy); > @@ -11107,7 +11056,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriverPtr driver, > ret = 0; > } > > - if (qemuGetProcessInfo(NULL, NULL, &rss, vm->pid, 0) < 0) { > + if (virProcessGetStat(vm->pid, 0, NULL, NULL, &rss) < 0) { > virReportError(VIR_ERR_OPERATION_FAILED, "%s", > _("cannot get RSS for domain")); > } else { > diff --git a/src/util/virprocess.c b/src/util/virprocess.c > index 1fbbbb3..98f4b25 100644 > --- a/src/util/virprocess.c > +++ b/src/util/virprocess.c > @@ -1426,3 +1426,65 @@ virProcessSetScheduler(pid_t pid ATTRIBUTE_UNUSED, > } > > #endif /* !HAVE_SCHED_SETSCHEDULER */ > + > +int > +virProcessGetStat(pid_t pid, int tid, > + unsigned long long *cpuTime, > + int *lastCpu, long *vm_rss) > +{ > + char *proc; > + FILE *pidinfo; > + 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 > + * is specific to Linux where int works fine. */ > + if (tid) > + ret = virAsprintf(&proc, "/proc/%d/task/%d/stat", (int) pid, tid); > + else > + ret = virAsprintf(&proc, "/proc/%d/stat", (int) pid); > + if (ret < 0) > + return -1; > + > + 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 (!pidinfo || > + fscanf(pidinfo, > + /* pid -> stime */ > + "%*d (%*[^)]) %*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"); > + } > + > + /* We got jiffies > + * We want nanoseconds > + * _SC_CLK_TCK is jiffies per second > + * So calculate thus.... > + */ > + if (cpuTime) > + *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime) > + / (unsigned long long)sysconf(_SC_CLK_TCK); > + if (lastCpu) > + *lastCpu = cpu; > + > + if (vm_rss) > + *vm_rss = rss * virGetSystemPageSizeKB(); > + > + > + 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); > + > + return 0; > + > +} > diff --git a/src/util/virprocess.h b/src/util/virprocess.h > index 3c5a882..2a2b91d 100644 > --- a/src/util/virprocess.h > +++ b/src/util/virprocess.h > @@ -106,4 +106,8 @@ typedef enum { > > int virProcessNamespaceAvailable(unsigned int ns); > > +int virProcessGetStat(pid_t pid, int tid, > + unsigned long long *cpuTime, > + int *lastCpu, long *vm_rss); > + > #endif /* __VIR_PROCESS_H__ */ > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
