Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On Fri, 7 Jan 2022, Ján Tomko wrote:
On a Friday in 2022, Ani Sinha wrote:
> On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > 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.
>
I don't see a !proc error path in virProcessGetSchedInfo.
if (tid)
proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
tid);
else
proc = g_strdup_printf("/proc/%d/sched", (int) pid);
if (!proc)
return -1; <=== not reported
The !lines case is inconsistent but thankfully it can only happen
if /proc/%d/sched is empty.
> 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.
Errors should be reported by the function that has the context to
provide a helpful error message.
Correct. Most often it is the lowest level functions.
For some low-level helpers, we rely
on errno's and let the caller report a less-specific error -
either
out of laziness because (like above) there would have to be a buggy
kernel, or because most of the code paths are syscalls which set errno.
Jano
> 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.
>
> Now having done that, we should also fix the callers so that the callers
> are not overriding the narrower errors with broader ones.