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

Revision history for this message
Rick Harris (rconradharris) wrote :

lgtm, just a few minor-nits:

> 626 + print rule

Might be better to log here (or remove if now unnecessary)

> 227 + new_filter = self._modify_rules(current_lines,
> 228 + tables[table])

228 needs to be indented 1 space to the right.

> 238 + new_filter = filter(lambda l: binary_name not in l, current_lines)

Super-minor, but I'd lean against using a single letter variable name here. Aside from the fact that 'l' (ell) and '1' (one) look similar, I think it reads better if we use var name like 'line'. (Though, in most cases, I think 'i' and 'j' are fine for counting variables).

Of course this is personal preference, so, if you disagree here, feel free to disregard :)

> 245 + elif seen_chains == 1:

`seen_chains` is a boolean, so unless I'm misunderstanding, this should just be `elif seen_chains`. Related, perhaps that block should be rewritten as

  if seen_chains:
    <blah>
  else:
    <blah>

> 241 + for rules_index in range(len(new_filter)):

This might be clearer if rewritten as

  for rule_index, rule in enumerate(new_filter):

> + 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.

review: Needs Fixing

« Back to merge proposal