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

2011/2/22 Rick Harris <email address hidden>:
>> 245   +            elif seen_chains == 1:
> `seen_chains` is a boolean, so unless I'm misunderstanding, this should just be `elif seen_chains`.

Actually, it could just be rewritten as "else:" :) It's a leftover
from a point where seen_chains had 4 states (and was named
differently). Good catch, thanks.

Related, perhaps that block should be rewritten as

>
>  if seen_chains:
>    <blah>
>  else:
>    <blah>

I prefer it the way it is. It matches the order of the output from
iptables-save (which is first
a comment, then a specification of the table to which the following
output pertains, then the chains,
then the rules).

>> 241   +        for rules_index in range(len(new_filter)):
>
> This might be clearer if rewritten as
>
>  for rule_index, rule in enumerate(new_filter):

Done.

>> +        new_filter[rules_index:rules_index] = [str(rule) for rule in rules]
> `rules_index` may end up uninitialized if the len(new_filter) == 0. We should probably handle that case even if it's unlikely (or even impossible) in practice.

I now initialise rules_index to 0.

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