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.
>
