Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feedback #4

Closed
Thomasdezeeuw opened this issue Jun 3, 2019 · 0 comments

Comments

@Thomasdezeeuw
Copy link

commented Jun 3, 2019

Following tokio-rs/mio#966.

Stage 1

(https://github.com/pusateri/rslogd/blob/70a647d01b91e314701b563aaf8dbd4dcdab9f36/src/main.rs)

  • You want to return a Result from main, then you don't have to unwrap/expect results but can return them directly. Generally this is something we try to follow in examples.
  • Events really doesn't need to be that big, usually 128/256 will do, but it's not a big problem.
  • Maybe comment on the match of the event token when this could happen, or change it in an unreachable statement.
  • Might want to add to the comment on top that root is required because of the port < 1024, so add network capabilities will be enough.
  • Need to ignore WouldBlock errors when receiving from the UDP socket, and I suggest Interrupted errors as well.

Stage 2

(https://github.com/pusateri/rslogd/blob/6e89c1d46ee7b957813405b6f52204d578451232/src/main.rs)

I've only looked at main, not at the syslog module.

  • You can use std::str::from_utf8 converting a &[u8] to a str, but since you don't know if the buffer contains valid UTF-8 I wouldn't recommend converting it into a string at all.
  • You've registered the socket with edge triggers (stage 1 was with level trigger), which means you have to drain the socket (keep receiving until it returns a WouldBlock error. Otherwise you can miss incoming packets that arrive close to each other and only produce a single event.

Stage 3

(https://github.com/pusateri/rslogd/blob/f071b5e00f225b7616f085135c7619615744905d/src/main.rs)

  • L125, L140. Same as with the registering of the UDP socket, the TCP listener needs to be drained (accepting connections until hitting a WouldBlock error).
  • L128, L143. Why are you clone the streams after you accepted them? That doesn't seem to be necessary.
  • L156. You need to deal with sporadic events, which means that HashMap::get_mut could fail and that would mean the program would panic. Instead if no connection with the token can be found the event should be ignored.
  • L158. The comment that deregistering is not necessary is incorrect, since you're using clone on the TCP connection deregistering is required (mainly because epoll will do silly things otherwise).
  • L195 Need to ignore WouldBlock errors, and I suggest Interrupted errors as well.

Stage 4

(https://github.com/pusateri/rslogd/blob/b6db229daa0ed4e5390c8f0fd088371075d43ee2/src/main.rs)

I'm only looking at Mio related things, so I didn't look at the TLS/CLI side of things.

  • L376. Again need to deal with WouldBlock and Interrupted errors, I don't know if the TLS session wrapper is able to deal with that properly (it not being a real error and being able to continue to be used after).

Stage 5

(https://github.com/pusateri/rslogd/blob/7df9d8fa92d92df547025ebc115eed5df5752a2c/src/main.rs)

Seems no changes in regards to Mio.

Overall

Overall this looks good, nice to see a presentation about Mio.

pusateri added a commit that referenced this issue Jun 26, 2019

address stage2 comments from @Thomasdezeeuw in #4. Thanks!
Loop in edge triggered receive routine until EWOULDBLOCK.
Only print syslog message received if it was not parseable in syslog
format and it's UTF-8 for debugging.

@pusateri pusateri closed this in e05dbdc Jun 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.