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

Roger Peppe (rogpeppe) wrote :

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

https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewaller.go#newcode139
worker/firewaller/firewaller.go:139: case mt.changes <-
&machineUnitsChange{mt, change}:
On 2012/07/18 13:56:01, niemeyer wrote:
> What happens if mt.changes is closed?

> We have to keep that kind of concern in mind when working with
channels.

AFAICS mt.changes is the same channel as machineUnitsChanges, which
there's no reason to close ever IMO (although I see that it is closed in
fact - i'd remove that).
I suggest putting machineUnitsChanges inside the firewaller type, so
this line would be:
case mt.firewaller.machineUnitsChanges <- ...

which makes the mux logic more clear, i think.

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

« Back to merge proposal