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

Daniel P. Berrangé berrange at redhat.com
Fri Jan 21 14:20:08 UTC 2022


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 at redhat.com
> > > <mailto:mprivozn at 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 was the quickest option here. Figuring
out a correct long term solution can follow up later

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|




More information about the libvir-list mailing list