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

[Michal Prívozník]

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 at 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 at redhat.com>

Michal