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@xxxxxxxxxx > > > > > <mailto:mprivozn@xxxxxxxxxx>> 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. I'm not saying the patch didn't have a good motiviation behind what it was trying to achieve, but the end result was a unfortunately a regresion in the public API in libvirt that impacted our users. That clearly qualfies for being called 'broken'. 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 :|
