[systemd-devel] [PATCH] journal: Introduce journal-syslogd

[Zbigniew Jędrzejewski-Szmek]

On Fri, Apr 10, 2015 at 12:45:34PM +0200, Lennart Poettering wrote:
> On Thu, 09.04.15 23:43, Susant Sahani (susant at redhat.com) wrote:
> 
> >    This tiny daemon enables to pull journal entries and push to a UDP
> > multicast address in syslog RFC 5424 format. systemd-journal-syslogd
> > runs with own user systemd-journal-syslog. It starts running after
> > the network is up.
> 
> Hmm, before we merge this, one more thing: I think we need a more
> explanatory name for this. If we call it "systemd-journal-syslogd",
> then people might assume this might be invovled with or necessary for
> systemd *reading* syslog traffic. However, that's not what it does, it
> only *sends* syslog traffic. Hence maybe call it
> "systemd-journal-syslog-emitd" or so?

Yeah, I have to agree very much here: "journal-syslogd" name is too similar
to "syslogd", the original syslog daemon, and is bound to cause endless confusion.
Maybe we should go for journal-netlogd, reminiscent of netconsole,
which is actually a similar thing. s-j-s-emitd is a bit too long.

I see some typos in the patch... I'll reply separately.

Zbyszek

> 
> Also, how can we tap into the multicast traffic for this? Is there any
> better tool for this around, that is commonly available and used?
> 
> How did you test this?
> 
> > +        if (rename(temp_path, m->state_file) < 0) {
> > +                r = -errno;
> > +                goto finish;
> > +        }
> > +
> > +        free(temp_path);
> > +        temp_path = NULL;
> > +
> > + finish:
> > +        if (r < 0)
> > +                log_error_errno(r, "Failed to save state %s: %m", m->state_file);
> > +
> 
> The failure path should remove the file temp_path here. Add something like:
> 
> if (temp_path)
>         (void) unlink(temp_path);
> 
> 
> > +static int network_send(Manager *m, struct iovec *iovec, unsigned n_iovec) {
> > +        struct msghdr mh = {
> > +                .msg_iov = iovec,
> > +                .msg_iovlen = n_iovec,
> > +        };
> > +
> > +        assert(m);
> > +        assert(iovec);
> > +        assert(n_iovec > 0);
> > +
> > +        if (m->address.sockaddr.sa.sa_family == AF_INET) {
> > +                mh.msg_name = &m->address.sockaddr.sa;
> > +                mh.msg_namelen = sizeof(m->address.sockaddr.sa);
> 
> This actually looks wrong? This should be
> sizeof(m->address.sockaddr.in)?
> 
> > +        } else if (m->address.sockaddr.sa.sa_family == AF_INET6) {
> > +                mh.msg_name = &m->address.sockaddr.in6;
> 
> And this doesn't look right either, this should be
> &m->address.sockaddr.sa, otherwise this should generate a type error,
> no?
> 
> Lennart
> 
> -- 
> Lennart Poettering, Red Hat
>