Code review comment for lp:~christof-mroz/hipl/hipfw-esp-speedup

Hi Christof!

On Mon, Mar 28, 2011 at 3:46 PM, Christof Mroz <email address hidden> wrote:
> On Sun, 27 Mar 2011 21:54:13 +0000
> Stefan Götz <email address hidden> wrote:
>
>> > + * @return true if rule was valid and output was
>> > written,
>> > + * false otherwise
>>
>> [L] Again, when is a rule 'valid'?
>
> Explaining the input format in detail would be reciting the
> implementation, I think. It already states that rules set up by
> hip_fw_manage_esp_rule() are covered, which is the important info.

Yes, I agree. What I meant was: it is not obvious, that 'rule' == 'input'. Consequently, you can give this function perfectly valid output from 'iptables -nvL' and that function would still return false - simply because not all output lines from 'iptables -nvL' are rules. That, however does not become clear from the documentation. Why I'm such a PITA about documentation here is that it's fairly difficult to correctly interpret the documentation without having read the source code in this case. Something like '@return true if @a input could be parsed as an ESP rule and the output parameters were set to the according values, false otherwise' is all I need to understand this better.

>> [M] Reading and zeroing the packet counters should be done atomically
>> with a single command line invocation. Otherwise, a packet might
>> arrive in between and the ESP rule would be deleted even though it
>> was still active.
>
> Good point. Didn't find a way to zero out individual rules though.

Yepp, it's not possible for individual rules, but you can combine the '-L' option for iptables with the '-Z' option for a particular chain to read and reset the counters atomically.

>> [M] I find this confusing. First, the @todo says that this
>> functionality should not be implemented as is but via libiptc
>> instead. Then it says the implementation should stay the way it is.
>> So is this really a @todo? Or is the @todo the update to libnl_nft?
>
> The TODO applies to "de-uglify this", everything else is just a hint. Would you degrade it to a @note?
>
> Alternatively, we could also discuss the issue right now and get rid of that paragraph altogether. Controlling iptables via shell may not be pretty, but using libiptc instead would blow up the code considerably in most cases, without real benefit. The only counterexample (that I know of), where libiptc could atually simplify matters, would be the ESP speedups introduced in this branch.
>
> But pulling in a new lib because of that? Sounds like overkill.
> Also, I bet there are many scripts out there that depend on the current output format, so the netfilter team will be careful not to change it unnecessarily.
>
> Some months later, I'm not so sure anymore that iptables will be obsoleted in the near future.

Okay, now I have all the information :-) This should go into a @note in order to justify your decision for parsing iptables output so every other developer doesn't have to scratch their heads as to why this route was chosen. I don't think we need 'to do' anything about this code at all.

>> > + if (strncmp(bfr, "Chain", 5) == 0) {
>> > + chain_ok = (strncmp(bfr, "Chain HIPFW-INPUT", 17) ==
>> > 0) ||
>> > + (strncmp(bfr, "Chain HIPFW-OUTPUT", 18) ==
>> > 0) ||
>> > + (strncmp(bfr, "Chain HIPFW-FORWARD", 19) ==
> 0);
>>
>> [L] Would it make sense to let the 'iptables' command list only these
>> three chains? That would help with the atomic zeroing of the packet
>> counts, too.
>
> That would involve three separate popen() calls,

Why is that a bad thing? iptabels is invoked via popen() for each chain anyway in order to reset the counters. As mentioned above, reading and resetting the counters could be merged into a single popen() call per chain so this would actually reduce the number of popen() calls.

Stefan