Code review comment for lp:~themue/juju-core/go-worker-firewaller-machines

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode44
worker/firewaller/firewaller.go:44: defer close(machineUnitsChanges)
i think this line should go before the machinesWatcher stop, otherwise
we'll see eof on the channel before the firewaller has properly stopped.
maybe that doesn't matter; i'm not sure.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode52
worker/firewaller/firewaller.go:52: if err != nil {
On 2012/07/17 16:13:50, niemeyer wrote:
> I believe we want to execute the line below no matter what. The err is
nil, the
> MustErr is supposed to explode pointing out the inconsistency.

in fact, if there's EOF from the machinesWatcher, it implies that the
machines watcher has already stopped, so there's no need to call Stop.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode118
worker/firewaller/firewaller.go:118: changes: changes,
we could store this in firewaller to make it obvious that the changes
channel is shared between all machines.

https://codereview.appspot.com/6374069/