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

[Ján Tomko]

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

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 at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200720/824604d4/attachment-0001.sig>