[systemd-devel] [PATCH] journal: return -ECHILD after a fork
Shawn
shawnlandden at gmail.com
Wed Jul 17 11:05:38 PDT 2013
Hi!
On Tue, Jul 16, 2013 at 8:50 AM, Lennart Poettering
<lennart at poettering.net>wrote:
> On Wed, 26.06.13 19:55, Zbigniew Jędrzejewski-Szmek (zbyszek at in.waw.pl)
> wrote:
>
> > A few asserts are replaced with 'return -EINVAL'. I think that
> > assert should not be used to check argument in public functions.
> >
> > Fields in struct sd_journal are rearranged to make it less
> > swiss-cheesy.
> > ---
> > Does this look sound?
>
> Yupp! Looks great! Please commit!
>
Wouldn't this be better using pthread_atfork() to avoid all the getpid()
calls?
I made a new patch using a global g_forked, but realized it had a race:
create journal
fork()
create second journal
log to first journal object
> >
> > Zbyszek
> >
> > TODO | 5 +--
> > man/sd_journal_open.xml | 7 ++++
> > src/journal/journal-internal.h | 18 +++++-----
> > src/journal/sd-journal.c | 80
> +++++++++++++++++++++++++++++++++++++++---
> > 4 files changed, 93 insertions(+), 17 deletions(-)
> >
> > diff --git a/TODO b/TODO
> > index caba4e3..2968d18 100644
> > --- a/TODO
> > +++ b/TODO
> > @@ -68,7 +68,7 @@ Features:
> >
> > * document systemd-journal-flush.service properly
> >
> > -* chane systemd-journal-flush into a service that stays around during
> > +* change systemd-journal-flush into a service that stays around during
> > boot, and causes the journal to be moved back to /run on shutdown,
> > so that we don't keep /var busy. This needs to happen synchronously,
> > hence doing this via signals is not going to work.
> > @@ -76,9 +76,6 @@ Features:
> > * allow implementation of InaccessibleDirectories=/ plus
> > ReadOnlyDirectories=... for whitelisting files for a service.
> >
> > -* libsystemd-journal:
> > - - return ECHILD as soon as somebody tries to reuse a journal object
> across a fork()
> > -
> > * libsystemd-bus:
> > - default policy (allow uid == 0 and our own uid)
> > - enforce alignment of pointers passed in
> > diff --git a/man/sd_journal_open.xml b/man/sd_journal_open.xml
> > index d7ea8ff..b2f6f9e 100644
> > --- a/man/sd_journal_open.xml
> > +++ b/man/sd_journal_open.xml
> > @@ -131,6 +131,13 @@
> > can be rotated at any moment, and the opening of
> > specific files is inherently racy.</para>
> >
> > + <para><varname>sd_journal</varname> objects cannot be
> > + used in the child after a fork. Functions which take a
> > + journal object as an argument
> > + (<function>sd_journal_next()</function> and others)
> > + will return <constant>-ECHILD</constant> after a fork.
> > + </para>
> > +
> > <para><function>sd_journal_close()</function> will
> > close the journal context allocated with
> > <function>sd_journal_open()</function> or
> > diff --git a/src/journal/journal-internal.h
> b/src/journal/journal-internal.h
> > index 5b717f8..5bc6535 100644
> > --- a/src/journal/journal-internal.h
> > +++ b/src/journal/journal-internal.h
> > @@ -97,8 +97,6 @@ struct Directory {
> > };
> >
> > struct sd_journal {
> > - int flags;
> > -
> > char *path;
> >
> > Hashmap *files;
> > @@ -109,27 +107,29 @@ struct sd_journal {
> > JournalFile *current_file;
> > uint64_t current_field;
> >
> > - Hashmap *directories_by_path;
> > - Hashmap *directories_by_wd;
> > -
> > - int inotify_fd;
> > -
> > Match *level0, *level1, *level2;
> >
> > + pid_t original_pid;
> > +
> > + int inotify_fd;
> > unsigned current_invalidate_counter, last_invalidate_counter;
> > + usec_t last_process_usec;
> >
> > char *unique_field;
> > JournalFile *unique_file;
> > uint64_t unique_offset;
> >
> > + int flags;
> > +
> > bool on_network;
> > bool no_new_files;
> >
> > size_t data_threshold;
> >
> > - Set *errors;
> > + Hashmap *directories_by_path;
> > + Hashmap *directories_by_wd;
> >
> > - usec_t last_process_usec;
> > + Set *errors;
> > };
> >
> > char *journal_make_match_string(sd_journal *j);
> > diff --git a/src/journal/sd-journal.c b/src/journal/sd-journal.c
> > index 1e70739..81b0c13 100644
> > --- a/src/journal/sd-journal.c
> > +++ b/src/journal/sd-journal.c
> > @@ -50,6 +50,15 @@
> >
> > #define DEFAULT_DATA_THRESHOLD (64*1024)
> >
> > +static bool journal_pid_changed(sd_journal *j) {
> > + assert(j);
> > +
> > + /* We don't support people creating a journal object and
> > + * keeping it around over a fork(). Let's complain. */
> > +
> > + return j->original_pid != getpid();
> > +}
> > +
> > /* We return an error here only if we didn't manage to
> > memorize the real error. */
> > static int set_put_error(sd_journal *j, int r) {
> > @@ -209,6 +218,8 @@ _public_ int sd_journal_add_match(sd_journal *j,
> const void *data, size_t size)
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > if (!data)
> > return -EINVAL;
> > @@ -303,7 +314,10 @@ fail:
> > }
> >
> > _public_ int sd_journal_add_conjunction(sd_journal *j) {
> > - assert(j);
> > + if (!j)
> > + return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > if (!j->level0)
> > return 0;
> > @@ -321,7 +335,10 @@ _public_ int sd_journal_add_conjunction(sd_journal
> *j) {
> > }
> >
> > _public_ int sd_journal_add_disjunction(sd_journal *j) {
> > - assert(j);
> > + if (!j)
> > + return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > if (!j->level0)
> > return 0;
> > @@ -391,7 +408,6 @@ char *journal_make_match_string(sd_journal *j) {
> > }
> >
> > _public_ void sd_journal_flush_matches(sd_journal *j) {
> > -
> > if (!j)
> > return;
> >
> > @@ -870,6 +886,8 @@ static int real_journal_next(sd_journal *j,
> direction_t direction) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > HASHMAP_FOREACH(f, j->files, i) {
> > bool found;
> > @@ -922,6 +940,8 @@ static int real_journal_next_skip(sd_journal *j,
> direction_t direction, uint64_t
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > if (skip == 0) {
> > /* If this is not a discrete skip, then at least
> > @@ -962,6 +982,8 @@ _public_ int sd_journal_get_cursor(sd_journal *j,
> char **cursor) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!cursor)
> > return -EINVAL;
> >
> > @@ -1001,6 +1023,8 @@ _public_ int sd_journal_seek_cursor(sd_journal *j,
> const char *cursor) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (isempty(cursor))
> > return -EINVAL;
> >
> > @@ -1100,6 +1124,8 @@ _public_ int sd_journal_test_cursor(sd_journal *j,
> const char *cursor) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (isempty(cursor))
> > return -EINVAL;
> >
> > @@ -1178,6 +1204,8 @@ _public_ int sd_journal_test_cursor(sd_journal *j,
> const char *cursor) {
> > _public_ int sd_journal_seek_monotonic_usec(sd_journal *j, sd_id128_t
> boot_id, uint64_t usec) {
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > reset_location(j);
> > j->current_location.type = LOCATION_SEEK;
> > @@ -1191,6 +1219,8 @@ _public_ int
> sd_journal_seek_monotonic_usec(sd_journal *j, sd_id128_t boot_id, u
> > _public_ int sd_journal_seek_realtime_usec(sd_journal *j, uint64_t
> usec) {
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > reset_location(j);
> > j->current_location.type = LOCATION_SEEK;
> > @@ -1203,6 +1233,8 @@ _public_ int
> sd_journal_seek_realtime_usec(sd_journal *j, uint64_t usec) {
> > _public_ int sd_journal_seek_head(sd_journal *j) {
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > reset_location(j);
> > j->current_location.type = LOCATION_HEAD;
> > @@ -1213,6 +1245,8 @@ _public_ int sd_journal_seek_head(sd_journal *j) {
> > _public_ int sd_journal_seek_tail(sd_journal *j) {
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > reset_location(j);
> > j->current_location.type = LOCATION_TAIL;
> > @@ -1651,6 +1685,7 @@ static sd_journal *journal_new(int flags, const
> char *path) {
> > if (!j)
> > return NULL;
> >
> > + j->original_pid = getpid();
> > j->inotify_fd = -1;
> > j->flags = flags;
> > j->data_threshold = DEFAULT_DATA_THRESHOLD;
> > @@ -1812,6 +1847,8 @@ _public_ int
> sd_journal_get_realtime_usec(sd_journal *j, uint64_t *ret) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!ret)
> > return -EINVAL;
> >
> > @@ -1838,6 +1875,8 @@ _public_ int
> sd_journal_get_monotonic_usec(sd_journal *j, uint64_t *ret, sd_id12
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > f = j->current_file;
> > if (!f)
> > @@ -1904,6 +1943,8 @@ _public_ int sd_journal_get_data(sd_journal *j,
> const char *field, const void **
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!field)
> > return -EINVAL;
> > if (!data)
> > @@ -2030,6 +2071,8 @@ _public_ int sd_journal_enumerate_data(sd_journal
> *j, const void **data, size_t
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!data)
> > return -EINVAL;
> > if (!size)
> > @@ -2080,6 +2123,8 @@ _public_ int sd_journal_get_fd(sd_journal *j) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > if (j->inotify_fd >= 0)
> > return j->inotify_fd;
> > @@ -2107,6 +2152,8 @@ _public_ int sd_journal_get_events(sd_journal *j) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > fd = sd_journal_get_fd(j);
> > if (fd < 0)
> > @@ -2120,6 +2167,8 @@ _public_ int sd_journal_get_timeout(sd_journal *j,
> uint64_t *timeout_usec) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!timeout_usec)
> > return -EINVAL;
> >
> > @@ -2220,6 +2269,8 @@ _public_ int sd_journal_process(sd_journal *j) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > j->last_process_usec = now(CLOCK_MONOTONIC);
> >
> > @@ -2258,7 +2309,10 @@ _public_ int sd_journal_wait(sd_journal *j,
> uint64_t timeout_usec) {
> > int r;
> > uint64_t t;
> >
> > - assert(j);
> > + if (!j)
> > + return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > if (j->inotify_fd < 0) {
> >
> > @@ -2307,6 +2361,8 @@ _public_ int
> sd_journal_get_cutoff_realtime_usec(sd_journal *j, uint64_t *from,
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!from && !to)
> > return -EINVAL;
> > if (from == to)
> > @@ -2348,6 +2404,8 @@ _public_ int
> sd_journal_get_cutoff_monotonic_usec(sd_journal *j, sd_id128_t boot
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!from && !to)
> > return -EINVAL;
> > if (from == to)
> > @@ -2405,6 +2463,8 @@ _public_ int sd_journal_get_usage(sd_journal *j,
> uint64_t *bytes) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!bytes)
> > return -EINVAL;
> >
> > @@ -2426,6 +2486,8 @@ _public_ int sd_journal_query_unique(sd_journal
> *j, const char *field) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (isempty(field))
> > return -EINVAL;
> > if (!field_is_valid(field))
> > @@ -2450,6 +2512,8 @@ _public_ int
> sd_journal_enumerate_unique(sd_journal *j, const void **data, size_
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!data)
> > return -EINVAL;
> > if (!l)
> > @@ -2562,6 +2626,8 @@ _public_ void sd_journal_restart_unique(sd_journal
> *j) {
> > _public_ int sd_journal_reliable_fd(sd_journal *j) {
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > return !j->on_network;
> > }
> > @@ -2595,6 +2661,8 @@ _public_ int sd_journal_get_catalog(sd_journal *j,
> char **ret) {
> >
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!ret)
> > return -EINVAL;
> >
> > @@ -2632,6 +2700,8 @@ _public_ int
> sd_journal_get_catalog_for_message_id(sd_id128_t id, char **ret) {
> > _public_ int sd_journal_set_data_threshold(sd_journal *j, size_t sz) {
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> >
> > j->data_threshold = sz;
> > return 0;
> > @@ -2640,6 +2710,8 @@ _public_ int
> sd_journal_set_data_threshold(sd_journal *j, size_t sz) {
> > _public_ int sd_journal_get_data_threshold(sd_journal *j, size_t *sz) {
> > if (!j)
> > return -EINVAL;
> > + if (journal_pid_changed(j))
> > + return -ECHILD;
> > if (!sz)
> > return -EINVAL;
> >
>
>
> Lennart
>
> --
> Lennart Poettering - Red Hat, Inc.
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
--
---
Shawn Landden
+1 360 389 3001 (SMS preferred)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20130717/660f0c61/attachment-0001.html>
More information about the systemd-devel
mailing list