search

Geal / rust-syslog

Send syslog messages from Rust
MIT License
110 stars 55 forks source link

Dependency upgrades #28

Closed bkchr closed 6 years ago

bkchr commented 6 years ago

Fixes: https://github.com/Geal/rust-syslog/issues/27

bkchr commented 6 years ago

I think that somehow broke something. My test machine logs everything with kern_emerg and almost everything in a new line. I will need to further debug this..

bkchr commented 6 years ago

Okay, I could fix the problem. It was not my fault, it was already present. We should create some tests to cover all this. @Geal are you willing to merge this?

daboross commented 6 years ago

IIRC format!() is really just write!() to a allocated string which then can be written with. This has a small performance penalty, and also seems a bit like a workaround rather than an actual fix. Do you think it'd be alright to separate that bugfix out into a separate PR for separate review/merging?

Both format!() / .write() and write!() will end up calling the exact same methods with the exact same bytes; the only difference I know of is that format!() allows batching things together. If format!() fixes the bug, it sounds like the actual bug is in LoggerBackend's Write implementation, which doesn't treat bytes equally.

I'm all for upgrading the dependencies, but it would be nice to be able to discuss and fix the bugfix separately from this main change.

bkchr commented 6 years ago

Yeah, I know that format! and write! should go almost the same path. The only difference I can imagine is that write! writes byte by byte or something similar and the syslog implementation reads it byte by byte. On my test machine I saw that each log message got split across several log messages. I can create a different pull request, just with "the fix"/hack in it. I'm open to a better solution, but currently I don't have any :/

Geal commented 6 years ago

Hello,

Thank you for taking care of this. First things first: please keep the changes minimal, avoid cosmetic changes and reindentation. I'd be willing to merge it but I do not have the time to check all the indentation changes and see if something else changed.

The fixes to kern_emerg and format! usage can go in other pull requests

On Feb 28, 2018 09:31, "Bastian Köcher" notifications@github.com wrote:

Yeah, I know that format! and write! should go almost the same path. The only difference I can imagine is that write! writes byte by byte or something similar and the syslog implementation reads it byte by byte. On my test machine I saw that each log message got split across several log messages. I can create a different pull request, just with "the fix"/hack in it. I'm open to a better solution, but currently I don't have any :/

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Geal/rust-syslog/pull/28#issuecomment-369159238, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHSAAz9_3cwD4_E163xYimQQVbodEKpks5tZQ7VgaJpZM4STU1I .

bkchr commented 6 years ago

@Geal yeah, I already created a new pull request. And yeah, I run rustfmt, but I created a separate commit for this, so the real changes can be seen easily.

oherrala commented 6 years ago

This PR also fixes #32.

bkchr commented 6 years ago

@Geal any chance that this could be merged? I also would be open to help you maintaining this crate.

Geal commented 6 years ago

Hello,

I merged all the commits except the rustfmt one. I'll see if I can make a format pass before the release. @bkchr I'll give you commit rights to the repo, thanks for your help :)