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