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@xxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
Michal
