On Fri, Jan 21, 2022 at 08:39:47PM +0530, Ani Sinha wrote:
On Fri, Jan 21, 2022 at 20:33 Daniel P. Berrangé
<berrange(a)redhat.com>
wrote:
> 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(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".
>
> 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.
No I did not read the bug report because it required me to create an
account and then login. I do not have my redhat BZ account credentials
handy.
Urgh, sorry I missed that the BZ was private. Private BZs are not
supposed to be listed in commit messages for precisely the reason
that people can't see them. The commit message should have showed
the problem listed in the private BZ description instead.
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 :|