[libvirt] [PATCH 1/2] util: rename qemuGetProcessInfo to virProcessGetStat
John Ferlan
jferlan at redhat.com
Mon Jun 12 11:29:10 UTC 2017
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__ */
>
More information about the libvir-list
mailing list