On a Saturday in 2020, Laine Stump wrote:
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.
Oh, I missed that it's called twice - I thought it was either one or the other call.
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.
Yeah, that would be wrong.
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".)JanoSigned-off-by: Laine Stump <laine@xxxxxxxxxx>My S-o-b stands. I still think this is the right thing to do.
S-o-b merely means that you are the author and/or have the author's permission to use the code. I don't think you can revoke a S-o-b, even if you don't think the code is right.
--- src/nwfilter/nwfilter_ebiptables_driver.c | 6 ------ 1 file changed, 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx> Jano
Attachment:
signature.asc
Description: PGP signature
