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

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(a)redhat.com&gt; 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(-) >>