[libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
laine at redhat.com
Sat Jul 18 04:25:50 UTC 2020
On 7/15/20 11:30 AM, Ján Tomko wrote:
> On a Tuesday in 2020, Laine Stump wrote:
>> On failure, this function would clear out and free the list of
>> subchains it had been called with. This is unnecessary, because the
>> *only* caller of this function will also clear out and free the list
>> of subchains if it gets a failure from ebtablesGetSubChainInsts().
>> (It also makes more logical sense for the function that is creating
>> the entire list to be the one freeing the entire list, rather than
>> having a function whose purpose is only to create *one item* on the
>> list freeing the entire list).
> This is the function creating the list,
I disagree with that characterization. The list is created, with 0
elements, when the caller (ebiptablesApplyNewRules()) defines it. Then
each time ebtablesGetSubChainInsts() is called, it doesn't create the
list anew, it just adds to whatever is already on the existing list - as
a matter of fact it is called multiple times and each time it adds more
items to the list without re=initializing it.
This is very much like what happens with a virBuffer - some function
creates a virBuffer by defining it and initializing it to empty, then
each time a virBuffer function is called, it adds more text to the
buffer. But if there is an error in a virBuffer function, it doesn't
clear out the buffer before returning, it just returns an error leaving
the buffer in whatever state it was in when the error occurred; it is
then up to the caller, who is the owner of the virBuffer, to clear it out.
> I think it makes sense
> to not leave anything allocated in case of failure.
Aside from making the code simpler and cleaner, I think it doesn't make
sense for one invocation of the function to clear out anything that was
put into the list by *a different* invocation of the function. If you're
going to be a purist about it, then a failed ebtablesGetSubChainInsts()
should remove from the list *only those items that were added during the
current call* and nothing else.
But that's just pedantic nitpicking (Hey, *you* started the nitpicking
(Also, there is only one caller of ebtablesGetSubChainInsts(), and
whenever ebtablesGetSubChainInsts() fails, the *very next thing* that
caller does is to clear out the entire list. So in fact, "nothing is
left allocated in case of failure".)
>> Signed-off-by: Laine Stump <laine at redhat.com>
My S-o-b stands. I still think this is the right thing to do.
>> src/nwfilter/nwfilter_ebiptables_driver.c | 6 ------
>> 1 file changed, 6 deletions(-)
More information about the libvir-list