On Thu, Oct 12, 2017 at 09:28:24PM -0400, John Ferlan wrote: > > > On 10/12/2017 08:48 AM, Erik Skultety wrote: > > On Tue, Oct 10, 2017 at 04:20:06PM -0400, John Ferlan wrote: > >> Rather than separate callbacks for the NumOfNetworks, GetNames, and > >> Export functions, let's combine them all into one multi-purpose callback. > >> > >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > >> --- > > > > NACK to the idea of consolidating all of them into a single callback, making it > > ambiguous, much harder to read and unmaintainable (not that we're about to > > introduce more getters for which another combination of elements within > > virNetworkObjForEachData would determine the action, but still...) > > First off - I'm OK w/ the NACK. Still this isn't unlike some of the > virdomainobjlist code... Still having a separate callback for each is > something I went with originally. Then going through the domainobj code > recently made me reconsider my decision. Also there's the virobject > patches I've had on list for a while which could do the combined callback. I share the same view on the fact that this patch creates one huge and complex callback that does several different things based on what members of the _virNetworkObjForEachData struct are set. This can easily backfire on us in the future if we need to change something or add a new code. I prefer simplicity over complexity because it's easier to maintain and understand for new contributors or even for core developers. For each usage of virHashForEach() function I would prefer having dedicated pair of callback and structure tailored to that exact usage. The reason why I prefer it is that you don't need to remember which members of the one huge structure were set for that specific usage while you are going through the callback to figure out what it actually does or when you are debugging something and so on. NACK to this change. Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
