[libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()

[Laine Stump]

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 
though :-P)


(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".)


>
> Jano
>
>>
>> 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(-)
>>