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

[Lennart Poettering]

On Wed, 18.03.15 11:00, Susant Sahani (susant at redhat.com) wrote:

Sorry for the late review!


> +    <para><command>systemd-journal-syslogd</command> serves journal
> +    events over the network. It multicasts journal event to Syslog RFC 5424 format.
> +    </para>

The tool can also be used to unicast events, no? Maybe clarify that it
can do unicasting as well as multicasting, it's just a matter of
specifying the right target address, no?

> +static int update_cursor_state(Manager *m) {
> +        _cleanup_fclose_ FILE *f = NULL;
> +        int r;
> +
> +        assert(m);
> +
> +        if (!m->state_file || !m->last_cursor)
> +                return 0;
> +
> +        f = fopen(m->state_file, "we");
> +        if (!f)
> +                goto finish;

I think this really should be written in "atomic" style, i.e. into a
temporary file first, that is then renamed into the actual state file
name. That way the state file is either the old or the new one, but
never half-written for other processes.

Our fopen_temporary() call helps wth this.

> +
> +        fprintf(f,
> +                "# This is private data. Do not parse.\n"
> +                "LAST_CURSOR=%s\n",
> +                m->last_cursor);
> +
> +        fflush(f);
> +
> +        if (ferror(f))
> +                r = -errno;

This flusing and check should be done via fflush_and_check().

> +static int manager_journal_event_handler(sd_event_source *event, int fd, uint32_t revents, void *userp) {
> +        Manager *m = userp;
> +        int r;
> +
> +        if (revents & EPOLLHUP) {
> +                log_debug("Received HUP");
> +                return 0;
> +        }
> +
> +        if (!(revents & EPOLLIN)) {
> +                log_warning("Unexpected poll event %"PRIu32".", revents);
> +                return -EINVAL;
> +        }
> +
> +        if (m->event_journal_input) {

Hmm, why this check? Isn't this set anyway if we entered this event
handler function?

> +
> +        r = sd_event_default(&m->event);
> +        if (r < 0)
> +                return log_error_errno(r, "sd_event_default failed: %m");
> +
> +        assert_se(sigemptyset(&mask) == 0);
> +        sigset_add_many(&mask, SIGINT, SIGTERM, -1);
> +        assert_se(sigprocmask(SIG_SETMASK, &mask, NULL) == 0);

We have sigprocmask_many() now for this (and shouild probably convert
all invocations like this to it...

> +
> +        pri = (uint16_t) strtoul(priority, NULL, 0);
> +        fac = (uint16_t) strtoul(facility, NULL, 0);

Hmm, I'd really like some error checking for this.

Also can we use safe_atou16() for this?

> +        r = setsockopt(m->socket, IPPROTO_IP, IP_PKTINFO, &one, sizeof(one));
> +        if (r < 0) {
> +                r = -errno;
> +                goto fail;
> +        }

This is not needed is it?

> +
> +        r = setsockopt(m->socket, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl));
> +        if (r < 0) {
> +                r = -errno;
> +                goto fail;
> +        }

And this neither?

> +
> +        r = setsockopt(m->socket, IPPROTO_IP, IP_MULTICAST_LOOP, &one, sizeof(one));
> +        if (r < 0) {
> +                r = -errno;
> +                goto fail;
> +        }

This might be useful.

Looks pretty good already!

Lennart

-- 
Lennart Poettering, Red Hat