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 errorI 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. The !lines case is inconsistent but thankfully it can only happen if /proc/%d/sched is empty.
hence thenon-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. 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.
Attachment:
signature.asc
Description: PGP signature
