On Fri, Jan 21, 2022 at 08:16:01PM +0530, Ani Sinha wrote:
>
>
> 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@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".

Did you actually read the bug report in the first message in this
series ? An end user hit a behaviour regression breaking the
installation process, and was caused by the patch that's been
reverted.