Re: [PATCH 1/6] conf, passt: Don't try to log to stderr after we close it

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 19 Jun 2024 12:06:01 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <sbrivio@redhat.com>    
> > > 
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > >   
> > > > ---
> > > >  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.

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:

	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.

-- 
Stefano