On Wed, 18.03.15 11:00, Susant Sahani (sus...@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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
