Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"

On Fri, 21 Jan 2022, Daniel P. Berrangé wrote: > On Fri, Jan 21, 2022 at 07:38:35PM +0530, Ani Sinha wrote: > > > > > > On Fri, 21 Jan 2022, Michal Prívozník wrote: > > > > > On 1/20/22 18:23, Ani Sinha wrote: > > > > > > > > > > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník <mprivozn(a)redhat.com > > > > <mailto:mprivozn@redhat.com>> wrote: > > > > > > > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > > > > > > > AKA kicking the can one more time 🙃 > > > > > > > > Well, I should have been more careful and not merge the patch in the > > > > first place. Changing API behavior is something we should never do. > > > > > > > > Looking at the code closer, it looks like all callers of this function > > > > would need to ignore the reported error so that their behavior is not > > > > changed. At this point, does it make sense to report an error in the > > > > function? > > > > > > > > > > > > The callers can decide what do with the error raised by the function. We > > > > should not write functions that cannot fail. > > > > > > > > > > But that's not what the commit does, is it. It changed some public APIs > > > from best effort to fail early. > > > > That is the side effect and I consider it as a bug. Imagine we add some > > more code into that function tomorrow and there is an error path. Now > > because of this bug, we will have to ignore the error condition and not > > report it. How ridiculous is that? > > > > We should have kept the patch as is and fix the callers so that the public > > APIs were not broken. > > That is not the libvirt approach. Our #1 priority is public API > compatibility, everything else is subservient to that. So when we > have a regression we fix that in the quickest possible way. Simply > reverting the broken patch> (a) I take exception to the fact that the patch reverted was "broken".