[systemd-devel] [PATCH] journalctl: quit on I/O error

[Lennart Poettering]

On Tue, 15.01.13 21:36, David Herrmann (dh.herrmann at googlemail.com) wrote:

> >> > +++ b/src/journal/journalctl.c
> >> > @@ -1077,7 +1077,7 @@ int main(int argc, char *argv[]) {
> >> >                                  arg_catalog * OUTPUT_CATALOG;
> >> >
> >> >                          r = output_journal(stdout, j, arg_output, 0, flags);
> >> > -                        if (r < 0)
> >> > +                        if (r < 0 || ferror(stdout))
> >> >                                  goto finish;
> >> >
> >> >                          need_seek = true;
> > Hm, is this the proper place to check for errors? Shouldn't the journal
> > output functions check for errors instead?
> 
> I don't think you win much by forwarding the errors all the way up.
> Other applications like this normally depend on SIGPIPE to be sent and
> then simply quit. That also works for journalctl but only if SIGPIPE
> is not ignored.
> So I could have added signal(SIGPIPE, SIG_DFL), but I thought ferror()
> is the nicer way.
> I also think we don't care whether output_journal() actually wrote the
> stuff, that's why there is no error checking. And this patch is also
> only useful to avoid having journalctl linger in the background and
> occupying the shell while the output is dead.
> Well, just my opinion, but feel free to catch the error inside of
> output_journal().

I'd side with David her. Checking for errors on stdout constantly will
make the code really ugly. I think in general we should avoid this
entirely, except when the output code is in a potentially long lasting
loop. It's actually kinda nice that FILE* objects cache the error for
us, so that we can check them much later. Given that the output
functions don't know that they are called in a loop I think it's a
pretty good solution to place a single check in the bigger outer
loop. It doesn't litter the code, is check "quickly enough" to be
effective, and does the job. That's why I merged it.

(Regarding the comment on signals: I am not a fan of signals. In systemd
code we generally avoid installing signal handlers if we can. Best way
to handle signals is via signalfd.)

Lennart

-- 
Lennart Poettering - Red Hat, Inc.