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 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 :|
