Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On 1/7/22 09:34, Ani Sinha wrote:
On Fri, 7 Jan 2022, Michal Prívozník wrote:
> On 1/7/22 09:05, Ani Sinha wrote:
>>
>>
>> On Fri, 7 Jan 2022, Michal Privoznik wrote:
>>
>>> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
>>> Linux centric. Provide stubs for non-Linux platforms.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> src/util/virprocess.c | 24 ++++++++++++++++++++++++
>>> 1 file changed, 24 insertions(+)
>>>
>>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>>> index c74bd16fe6..5788faea9c 100644
>>> --- a/src/util/virprocess.c
>>> +++ b/src/util/virprocess.c
>>> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
>>> }
>>>
>>>
>>> +#ifdef __linux__
>>> int
>>> virProcessGetStatInfo(unsigned long long *cpuTime,
>>> int *lastCpu,
>>> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
>>>
>>> return 0;
>>> }
>>> +
>>> +#else
>>> +int
>>> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
>>> + int *lastCpu G_GNUC_UNUSED,
>>> + long *vm_rss G_GNUC_UNUSED,
>>> + pid_t pid G_GNUC_UNUSED,
>>> + pid_t tid G_GNUC_UNUSED)
>>> +{
>>> + errno = ENOSYS;
>>> + return -1;
>>> +}
>>> +
>>> +int
>>> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
>>> + pid_t pid G_GNUC_UNUSED,
>>> + pid_t tid G_GNUC_UNUSED)
>>> +{
>>> + virReportSystemError(ENOSYS, "%s",
>>> + _("scheduler information is not supported on
this platform"));
>>
>> We should do this for both functions for non-linux platforms or not do it
>> at all. Like it should be consistent IMHO.
>
> I don't think so. Just like we've discussed under one patch of yours, a
> function should either report error in all cases or none. And in case of
> virProcessGetSchedInfo() the linux version does report error
I see your point but there is also a bug in that function - not all error
paths report errors. For example, !proc and !lines cases. We need to fix
that.
D'oh! I knew I forgot something in the other patch I've sent today:
https://listman.redhat.com/archives/libvir-list/2022-January/msg00270.html
g_strsplit() won't return NULL on sensible inputs (i.e. !NULL). Let me
rerun my spatch and post another patch.
hence the
> non-linux variant should report an error too. And in case of
> virProcessGetStatInfo() no error is reported for linux version thus
> non-linux version shouldn't report an error.
>
No this is not my understanding from the discussion. What I understood is
that the lowest level of functions should always report error when an
error path is encountered. For example virFileReadAll() does this nicely.
Currently there is no error path in virProcessGetStatInfo() and it
uncondiotionally returns 0. For non-linux variant, I think it would be
correct to report an error.
Yes and no. Reporting error where it happens is okay most of the time
[1]. But having a function that's consistent in reporting or not
reporting an error in all cases is above that. I'm failing to see how
one can write effective code if a function reports error in some cases
but it doesn't in others. Especially if you want to report less generic
errors and more specific ones.
1: In a few cases it may be worth reporting more generic error because
it may be more user friendly and thus shed more light into what's going
on. But this situation is very rare.
Now having done that, we should also fix the callers so that the
callers
are not overriding the narrower errors with broader ones.
Agreed, see my reply to the patch you posted.
Michal