Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add syslog supports #2537
Add syslog supports #2537
Changes from 21 commits
ae86c6a
ad6a2f3
216b4a8
216911e
a19d932
953fdec
98289d5
f527072
ee8ffed
5c547d6
f4bd797
0e6c99b
f9df215
a60f314
d051dc6
12e7bdf
169b5d0
0c61a95
584e4df
718519c
c98a454
9f3596e
541801e
596280b
1cadebe
7014ca1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface should be good for UNIXs other than Linux, since Linux would store the
ident
pointer as-is (will be used during whole process lifetime), we need to add a separate interface for Linux, and gate this with#[cfg(not(target_os = "linux"))]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And thanks for adding the Option here! I didn't catch it in my first round of review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we remove the
ident
param for now.It's not easy to just add
Option<&'static str>
because it may not end withnul
and I don't know how to achieve this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't observe the
ident
on macos and linux, perhaps this is because the allocated string is collected 🤣There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
Option<&'static CStr>
. Let me try.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nix does not pursue uniform interfaces across OSes, if they are different, let them be different. I suggest keeping the original interface for non-Linux platforms as they are not constrained by this limit and adding a separate interface for Linux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you send a patch to this PR or just edit this PR with the suggested change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let me send a commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 596280b