[PATCH v3] 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. Also fix non-linux implementation of this function so as to report error. Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/ch/ch_driver.c | 2 -- src/qemu/qemu_driver.c | 7 +------ src/util/virprocess.c | 9 +++++++-- 3 files changed, 8 insertions(+), 10 deletions(-) changelog: v3: fix non-linux implementation v2: fix 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 d3d76c003f..9a17c93b08 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 b559a4257e..709ec616de 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1784,7 +1784,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 @@ -1881,7 +1885,8 @@ virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED, pid_t pid G_GNUC_UNUSED, pid_t tid G_GNUC_UNUSED) { - errno = ENOSYS; + virReportSystemError(ENOSYS, "%s", + _("Process statistics data is not supported on this platform")); return -1; } -- 2.25.1
On 1/11/22 11:20, 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. Also fix non-linux implementation of this function so as to report error.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/ch/ch_driver.c | 2 -- src/qemu/qemu_driver.c | 7 +------ src/util/virprocess.c | 9 +++++++-- 3 files changed, 8 insertions(+), 10 deletions(-)
changelog: v3: fix non-linux implementation v2: fix callers
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3d76c003f..9a17c93b08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
@@ -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;
Returning -1 changes semantics of this function. Previously it was tolerant to errors and now it isn't. For instance, if this function was called on a FreeBSD machine (yes, QEMU driver can be enabled there) then despite fetching mem stats earlier through monitor (not visible in the context) an error is now returned which in turn makes whole virDomainMemoryStats() API fail. The 'return -1' should be replaced with virResetLastError(). Having said that, now there will be an error reported in the logs every time the API is called. I wonder what we can do about it. Previously it was "just" a warning. Michal
On Tue, 11 Jan 2022, Michal Prívozník wrote:
On 1/11/22 11:20, 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. Also fix non-linux implementation of this function so as to report error.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/ch/ch_driver.c | 2 -- src/qemu/qemu_driver.c | 7 +------ src/util/virprocess.c | 9 +++++++-- 3 files changed, 8 insertions(+), 10 deletions(-)
changelog: v3: fix non-linux implementation v2: fix callers
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3d76c003f..9a17c93b08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
@@ -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;
Returning -1 changes semantics of this function. Previously it was tolerant to errors and now it isn't. For instance, if this function was called on a FreeBSD machine (yes, QEMU driver can be enabled there) then despite fetching mem stats earlier through monitor (not visible in the context) an error is now returned which in turn makes whole virDomainMemoryStats() API fail.
The 'return -1' should be replaced with virResetLastError().
Having said that, now there will be an error reported in the logs every time the API is called. I wonder what we can do about it. Previously it was "just" a warning.
Does v4 help?
On 1/11/22 19:14, Ani Sinha wrote:
On Tue, 11 Jan 2022, Michal Prívozník wrote:
On 1/11/22 11:20, 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. Also fix non-linux implementation of this function so as to report error.
Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/ch/ch_driver.c | 2 -- src/qemu/qemu_driver.c | 7 +------ src/util/virprocess.c | 9 +++++++-- 3 files changed, 8 insertions(+), 10 deletions(-)
changelog: v3: fix non-linux implementation v2: fix callers
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3d76c003f..9a17c93b08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
@@ -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;
Returning -1 changes semantics of this function. Previously it was tolerant to errors and now it isn't. For instance, if this function was called on a FreeBSD machine (yes, QEMU driver can be enabled there) then despite fetching mem stats earlier through monitor (not visible in the context) an error is now returned which in turn makes whole virDomainMemoryStats() API fail.
The 'return -1' should be replaced with virResetLastError().
Having said that, now there will be an error reported in the logs every time the API is called. I wonder what we can do about it. Previously it was "just" a warning.
Does v4 help?
Not really. Let me squash in the change I'm suggesting and merge it. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Ani Sinha -
Michal Prívozník