1. Patchew
  2. Libvirt
  3. View series

[PATCH v3] report error when virProcessGetStatInfo() is unable to parse data

Ani Sinha posted 1 patch 2 years, 3 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20220111102043.384822-1-ani@anisinha.ca
There is a newer version of this series
src/ch/ch_driver.c     | 2 --
src/qemu/qemu_driver.c | 7 +------
src/util/virprocess.c  | 9 +++++++--
3 files changed, 8 insertions(+), 10 deletions(-)
[PATCH v3] report error when virProcessGetStatInfo() is unable to parse data
Posted by Ani Sinha 2 years, 3 months ago 
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
Re: [PATCH v3] report error when virProcessGetStatInfo() is unable to parse data
Posted by Michal Prívozník 2 years, 3 months ago 
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
Re: [PATCH v3] report error when virProcessGetStatInfo() is unable to parse data
Posted by Ani Sinha 2 years, 3 months ago 

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?
Re: [PATCH v3] report error when virProcessGetStatInfo() is unable to parse data
Posted by Michal Prívozník 2 years, 3 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.