Re: [libvirt] [PATCH 2/2] network: Introduce virNetworkObjListForEachCb
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(a)redhat.com>
>> ---
>
> 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
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)