Code review comment for ~kzapalowicz/snappy-hwe-snaps/+git/udisks2:fix/fail-to-mount-after-unmount

Revision history for this message
Tony Espy (awe) wrote :

A couple of comments:

1) Commit cc57ba0 & 5273bbe both essentially do the same thing, they make a channel write operation to an unbuffered channel non-blocking, and discard whatever was to be sent. As such, I think the commit messages should look similar, but they're different. I'd suggest:

udisks2: use non-blocking channel writes

This commit makes sure that if the client does not subscribe to
the unmountCompleted channel, a write to it won't block, and the
event discarded.

dispatcher: use non-blocking channel writes

This commit makes sure that writes to the Additions or Jobs channel
won't block if there's no reader. In both cases the event is discarded.

2) You've essentially fixed the bug with unmountCompleted two ways. You first added non-blocking IO writes in udisks2.go, but then you changed the client in commit fa4c532 to uses SubscribeUnmountEvents. Note, you only added the non-blocking IO for writes to unmountCompleted, not unmountError. I would think that just fa4c532 alone would fix the problem, wouldn't it?

3) Is dropping events in the dispatcher the right thing to do? Don't these events correlate to DBus messages from udisks2? Under what conditions would these two channels (Jobs or Additions) not be read? What are the side effects of these events being dropped?

review: Needs Fixing

« Back to merge proposal