[libvirt PATCH v2 08/15] nwfilter: remove unnecessary code from ebtablesGetSubChainInsts()
jtomko at redhat.com
Mon Jul 20 21:04:06 UTC 2020
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
>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
>(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".)
>>>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>
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 488 bytes
Desc: not available
More information about the libvir-list