Geal / rust-syslog

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

Is tempfile needed? #1

Closed tailhook closed 9 years ago

tailhook commented 9 years ago

Hi,

I wonder why do you create temporary path and bind to it for connecting to unix datagram socket? (https://github.com/Geal/rust-syslog/blob/master/src/lib.rs#L123)

It seems that competitor sysly doesn't do that (https://github.com/softprops/sysly/blob/master/src/lib.rs#L154)

Geal commented 9 years ago

A common practice when talking to a Unix socket is to use a temporary file with an unpredictable name. I could provide a way to initialize the unix socket logger with a specified path, instead of handling it for the user. Would that be alright for you?

tailhook commented 9 years ago

A common practice when talking to a Unix socket is to use a temporary file with an unpredictable name

My actual questions is why you need to "bind" at all? AFAIU, you can just connect to the syslog socket.

Geal commented 9 years ago

The syslog unix socket is a datagram socket. For those, you also need to define an entry point. This puzzled me too when I implemented it :wink:

tailhook commented 9 years ago

The syslog unix socket is a datagram socket. For those, you also need to define an entry point. This puzzled me too when I implemented it :wink:

I'm not sure, are you saying that sysly (code linked above) does not work?

For example socat does with datagram sockets the following (strace output):

socket(PF_LOCAL, SOCK_DGRAM, 0)         = 5
sendto(5, "hello\n", 6, 0, {sa_family=AF_LOCAL, sun_path="/dev/log"}, 110) = 6

I.e. I think that address is autogenerated by OS, probably.

Geal commented 9 years ago

I do not know if sysly works. What I know is that when I tried stream sockets, on OSX and Linux, it did not work, while a datagram socket worked. I can add an option for stream unix sockets. It seems syslog-ng recommends stream sockets, but not all systems support it.

Geal commented 9 years ago

Could you try the two options on different systems and tell me which ones will require the stream socket?

tailhook commented 9 years ago

What I know is that when I tried stream sockets, on OSX and Linux, it did not work, while a datagram socket worked.

Ah, ok, just noticed that sysly opens streams socket. Anyway, socat does work with datagram sockets without bind anyway. Doesn't it work in OS X?

Could you try the two options on different systems and tell me which ones will require the stream socket?

Well, all my systems are linux. I could try on different ubuntus, arch, nix, but not on OS X, or FreeBSD.

tailhook commented 9 years ago

And it seems standard logger does not bind socket in both dgram and stream mode too: https://github.com/karelzak/util-linux/blob/master/misc-utils/logger.c#L264

Similarly python implementation: https://hg.python.org/cpython/file/3358b5d32910/Lib/logging/handlers.py#l815 (note: python implementation should be cross-platform, but, at least old versions of python for OS X, it was patched to have different default syslog address, can't find a link ATM)

Geal commented 9 years ago

Look, to be honest, I don't understand why you insist on this. The current code works. It could use connect() but it is not a crucial part of the library. The main difference here is that connect() restrict on one server, otherwise it works fine.

So if you can propose a patch to use connect(), that's great, I'll merge it, but I cannot spend the day arguing about this, sorry.

tailhook commented 9 years ago

Sure, I can do the patch. I'm insisting on it a little bit just because I don't want useless tmp files (in case process crashes), unless absolutely needed.

Geal commented 9 years ago

It makes sense. I'm waiting for your patch then :)

I'll test it on OSX as well.

tailhook commented 9 years ago

Ah, ok, I see your problem. I've just made pull request for rust-unix-socket: https://github.com/sfackler/rust-unix-socket/pull/14