On Tue, 15.01.13 21:36, David Herrmann ([email protected]) 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. _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
