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.