On Wed, Jun 19, 2024 at 10:13:46AM +0200, Stefano Brivio wrote: > On Wed, 19 Jun 2024 12:06:01 +1000 > David Gibson wrote: > > > On Tue, Jun 18, 2024 at 08:00:14AM +0200, Stefano Brivio wrote: > > > On Tue, 18 Jun 2024 10:36:28 +1000 > > > David Gibson wrote: > > > > > > > On Mon, Jun 17, 2024 at 02:03:14PM +0200, Stefano Brivio wrote: > > > > > If we don't run in foreground, we close standard error as we > > > > > daemonise, so it makes no sense to check if the controlling terminal > > > > > is an interactive terminal or if --force-stderr was given, to decide > > > > > if we want to log to standard error. > > > > > > > > > > Make --force-stderr depend on --foreground. > > > > > > > > > > Signed-off-by: Stefano Brivio > > > > > > > > Reviewed-by: David Gibson > > > > > > > > > --- > > > > > conf.c | 3 +++ > > > > > passt.c | 2 +- > > > > > 2 files changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/conf.c b/conf.c > > > > > index 94b3ed6..dbdbb62 100644 > > > > > --- a/conf.c > > > > > +++ b/conf.c > > > > > @@ -1693,6 +1693,9 @@ void conf(struct ctx *c, int argc, char **argv) > > > > > > > > > > conf_ugid(runas, &uid, &gid); > > > > > > > > > > + if (!c->foreground && c->force_stderr) > > > > > + die("Can't log to standard error if not running in foreground"); > > > > > + > > > > > if (logfile) { > > > > > logfile_init(c->mode == MODE_PASTA ? "pasta" : "passt", > > > > > logfile, logsize); > > > > > diff --git a/passt.c b/passt.c > > > > > index a5e2c5a..aa9648a 100644 > > > > > --- a/passt.c > > > > > +++ b/passt.c > > > > > @@ -302,7 +302,7 @@ int main(int argc, char **argv) > > > > > if (isolate_prefork(&c)) > > > > > die("Failed to sandbox process, exiting"); > > > > > > > > > > - if (!c.force_stderr && !isatty(fileno(stderr))) > > > > > + if (!c.foreground || (!c.force_stderr && !isatty(fileno(stderr)))) > > > > > > > > What's the rationale for the isatty() check in any case? > > > > > > To implement the behaviour from the man page: > > > > > > -e, --stderr > > > Log to standard error too. Default is to log to the sys‐ > > > tem logger only, if started from an interactive terminal, > > > and to both system logger and standard error otherwise. > > > > Hmm.. maybe I'm getting confused reading either the man page or the > > code, but isn't that the opposite of what the code is doing? The code > > appears to be opening the log if we're *not* on an interactive terminal. > > Ouch, right, this became the opposite of the original intended > behaviour from 84a62b79a2bc ("passt: Also log to stderr, don't fork to > background if not interactive") which again was implemented as a workaround > for issues in the old version of the KubeVirt integration that's not used > anymore. > > The swap happened in 1e49d194d017 ("passt, pasta: Introduce command-line > options and port re-mapping"), which makes me think that we should probably > think of a reasonable default and meaning of --stderr regardless of the > original workaround (which is surely not needed anymore) and implement it. I agree. > Probably it would make sense to log to standard error if we're running in > foreground, unless a log file is specified. The check above would simply > become: I think logging to stderr when foreground even if we do have a logfile makes even more sense. > if (!c.foreground) > > and we could accept -e, --stderr for compatibility only, but it wouldn't > do anything. > > > > which was needed, in turn, because of earlier versions of passt and of > > > the KubeVirt integration where passt was running in foreground, but (of > > > course) not attached to a terminal, and there was no option to force > > > printing to standard error. > > > > > > Given that KubeVirt doesn't have a system logger, it was otherwise > > > impossible to get messages out of passt. > > > > > > It's an abomination that just adds complexity now, and I don't think > > > anybody uses it that way anymore. I guess we should drop that, together > > > with qrap, in a few months from now. > > > > Right.. but what I'm after is the rationale for this depending on > > whether it's an interactive terminal at all. Seems to me the logical > > thing to do is to (by default) always log to stderr before > > daemonization (i.e. forever when in foreground mode), then to logfile > > and/or syslog after daemonization. Regardless of whether there's a > > tty or not. > > The original rationale was that if it was started from an interactive > terminal, I didn't want the whole spam associated with it, but if it was > started from a non-interactive terminal (KubeVirt integration), that was > the only way to get messages out. > > Then we had the opposite issue with KubeVirt (which is the reason why > the log file disables stderr logging): passt started in a > non-interactive terminal, but stderr logging would cause a lot of > overhead in some KubeVirt component. Again, KubeVirt can fix that with the Go equivalent of '2> /dev/null'. I don't think we should make our semantics more complex for the sake of that. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson