[PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
Currently virProcessGetStatInfo() always returns success and only logs error when it is unable to parse the data. Make this function actually report the error and return a negative value in this error scenario. Fix the callers so that they do not override the error generated. Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/ch/ch_driver.c | 2 -- src/qemu/qemu_driver.c | 7 +------ src/util/virprocess.c | 6 +++++- 3 files changed, 6 insertions(+), 9 deletions(-) changelog: v2: fixed the callers diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 53e0872207..3cbc668489 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm, if (virProcessGetStatInfo(&vcpuinfo->cpuTime, &vcpuinfo->cpu, NULL, vm->pid, vcpupid) < 0) { - virReportSystemError(errno, "%s", - _("cannot get vCPU placement & pCPU time")); return -1; } } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4974450333..015ffb2ce7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm, if (virProcessGetStatInfo(&vcpuinfo->cpuTime, &vcpuinfo->cpu, NULL, vm->pid, vcpupid) < 0) { - virReportSystemError(errno, "%s", - _("cannot get vCPU placement & pCPU time")); return -1; } } @@ -2521,8 +2519,6 @@ qemuDomainGetInfo(virDomainPtr dom, if (virDomainObjIsActive(vm)) { if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL, vm->pid, 0) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("cannot read cputime for domain")); goto cleanup; } } @@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver, } if (virProcessGetStatInfo(NULL, NULL, &rss, vm->pid, 0) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("cannot get RSS for domain")); + return -1; } else { stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS; stats[ret].val = rss; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index c74bd16fe6..b9f498d5d8 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime, virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 || virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { - VIR_WARN("cannot parse process status data"); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse process status data for pid '%d/%d'"), + (int) pid, (int) tid); + + return -1; } /* We got jiffies -- 2.25.1
On 1/11/22 04:42, Ani Sinha wrote:
Currently virProcessGetStatInfo() always returns success and only logs error when it is unable to parse the data. Make this function actually report the error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/ch/ch_driver.c | 2 -- src/qemu/qemu_driver.c | 7 +------ src/util/virprocess.c | 6 +++++- 3 files changed, 6 insertions(+), 9 deletions(-)
changelog: v2: fixed the callers
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index c74bd16fe6..b9f498d5d8 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime, virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 || virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { - VIR_WARN("cannot parse process status data"); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse process status data for pid '%d/%d'"), + (int) pid, (int) tid); + + return -1; }
/* We got jiffies
There's an alternative implementation for non-Linux platforms, but you are introducing error reporting only to this implementation. Both versions have to behave the same, i.e. the non-Linux implementation now has to report error too. Can you fix that in v3? Michal
On Tue, 11 Jan 2022, Michal Prívozník wrote:
On 1/11/22 04:42, Ani Sinha wrote:
Currently virProcessGetStatInfo() always returns success and only logs error when it is unable to parse the data. Make this function actually report the error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/ch/ch_driver.c | 2 -- src/qemu/qemu_driver.c | 7 +------ src/util/virprocess.c | 6 +++++- 3 files changed, 6 insertions(+), 9 deletions(-)
changelog: v2: fixed the callers
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index c74bd16fe6..b9f498d5d8 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime, virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 || virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { - VIR_WARN("cannot parse process status data"); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse process status data for pid '%d/%d'"), + (int) pid, (int) tid); + + return -1; }
/* We got jiffies
There's an alternative implementation for non-Linux platforms, but you
but that has not been pushed yet I believe. Correct me if I am mistaken.
are introducing error reporting only to this implementation. Both versions have to behave the same, i.e. the non-Linux implementation now has to report error too. Can you fix that in v3?
I think we should keep the scope of this patch as is. Why don't we do this. Lets push this patch. Then I will fix the patch for non-linux platforms and send it over.
On Tue, Jan 11, 2022 at 15:16:36 +0530, Ani Sinha wrote:
On Tue, 11 Jan 2022, Michal Prívozník wrote:
On 1/11/22 04:42, Ani Sinha wrote:
Currently virProcessGetStatInfo() always returns success and only logs error when it is unable to parse the data. Make this function actually report the error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/ch/ch_driver.c | 2 -- src/qemu/qemu_driver.c | 7 +------ src/util/virprocess.c | 6 +++++- 3 files changed, 6 insertions(+), 9 deletions(-)
changelog: v2: fixed the callers
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index c74bd16fe6..b9f498d5d8 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime, virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &systime) < 0 || virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { - VIR_WARN("cannot parse process status data"); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse process status data for pid '%d/%d'"), + (int) pid, (int) tid); + + return -1; }
/* We got jiffies
There's an alternative implementation for non-Linux platforms, but you
but that has not been pushed yet I believe. Correct me if I am mistaken.
It was pushed already as it was fixing the build, which had higher priority than any cosmetic issues which can be fixed later: commit d7c64453aa0e9e0718e9292d7269ea3fe34f7c4c Author: Michal Prívozník <mprivozn@redhat.com> Date: Thu Jan 6 20:13:08 2022 +0100 virprocess: Provide non-Linux stubs for virProcessGet{Stat,Sched}Info Both virProcessGetStatInfo() and virProcessGetSchedInfo() are Linux centric. Provide stubs for non-Linux platforms. Fixes: d73852c49962fbfe652fc7bec595a3f242faf847 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
participants (3)
-
Ani Sinha -
Michal Prívozník -
Peter Krempa