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

[Susant Sahani]

On 04/09/2015 02:02 PM, Lennart Poettering wrote:
> On Wed, 18.03.15 11:00, Susant Sahani (susant at redhat.com) wrote:
>
> Sorry for the late review!

Thanks for the 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?
>
yes infact I tested with the unicast . added now .

>> +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.
>
Ok

>> +
>> +        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().
>
Added.
>> +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?
>
OK
>> +
>> +        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...
>
OK
>> +
>> +        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?

Yes doing now .
>
>> +        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?

Removed both

>
>> +
>> +        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!
>
Thanks !
> Lennart
>

Susant