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