Code review comment for lp:~soren/nova/iptables-concurrency

2011/2/22 Todd Willey <email address hidden>:
> So nova-local is now used for FORWARD and OUTPUT, whereas before it was only used for FORWARD, correct?  Why the change?

This is the original code that is being replaced:

858 - our_chains += [':nova-local - [0:0]']
859 - our_rules += ['-A FORWARD -j nova-local']
860 - our_rules += ['-A OUTPUT -j nova-local']

So no change.

> Is use_nova_chains used anymore?  It seems to have been replaced with the 'wrap' parameter.

True. I've removed the flag now.

>  It may be prettier to have a helper function wrapped_chain('INPUT') or nova_chain('FORWARD') that we could use as the first parameter to add_rule, since we'd have the name in one spot, instead of having to look at two different parameters to determine what the actual chain name is.

Why/when do you have to look in two places to work this out?

I really like that consumers of this IptablesManager need not worry
about the the naming at all, but can just define whatever name they
want, and IptablesManager ensures that it doesn't collide with
anything else.

>  That also might let us get rid of the IptablesRule class and just store rules as tuples of (chain, rule).

I started out that way, actually, but ended up rewriting it like this.
I found it more readable this way.

> I'm not quite done with this review, but I wanted to give you my early comments.

Np. Thanks!

--
Soren Hansen        | http://linux2go.dk/
Ubuntu Developer    | http://www.ubuntu.com/
OpenStack Developer | http://www.openstack.org/