From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 78A765A004E for ; Wed, 19 Jun 2024 10:14:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1718784865; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0Cbk4gMLTPlX/0prO3UnofXszZlh3Vl8XlvbKWMF8JA=; b=cwbJTX+B6ozGsiNXJjbRCNXd2FuO5hTfIa5Ro/HEG7nBAtnK0lH8Jn8np5BpZ5BRyXvaMH Y9YhRcWBqlLdIdhDVNX8LrtCqeo94gdRYK7lLJa2Sk8p/mma3AwG89TmpxDTViKj/TzyZp Hs2i8+4/hky9OnyLlBTc6pKgHtGXGvQ= Received: from mail-yb1-f197.google.com (mail-yb1-f197.google.com [209.85.219.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-90-XMJbtmZJNryKl_qTIFM81A-1; Wed, 19 Jun 2024 04:14:23 -0400 X-MC-Unique: XMJbtmZJNryKl_qTIFM81A-1 Received: by mail-yb1-f197.google.com with SMTP id 3f1490d57ef6-dff16daff8dso9667990276.2 for ; Wed, 19 Jun 2024 01:14:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718784863; x=1719389663; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=nqtc8AEeiqPpfJb9KmdedrqPJzxMhfwe08JeIx8Ob2w=; b=IlAPiHOnb1SFNzgYl4Pdr19VYY367k5r0/+XUZv+c3qe1jaAJfO+lhNIH7UfmFBBj/ QpXfR1Zm6R+6mHL6B1iZjFbyanjy15xaUWCxfRz6BFunv5xD6Ktmp1YHnDrdZ88FBDcE Me99X0/X/2oEyNEvh4YoRo4lD7xeOXudqk2O7w+PxrY3CnyjyRcI4bjzKjbtNbK5NtFo VYxGYxSNzl/34y9mjb3HfGU5v84mBPc2VHHp0/lSXIW0avgz9tfqQbZvhVa4DlY8757u utW6KttM4QHZgwBSWNylK09O8WfVsDAf4irTjjzzNndEktZX1HsjpYU0BPKgd5NNhEmY Nsdw== X-Gm-Message-State: AOJu0YwElviJ5q+cCNzfU4JgXHtFVUtI2aK/ED9iu3jO1fEVA+UVO3LD FXVXlNRF1UVaCQehVoVmXkFdS0sF1RP0M4gHa2kg3p5zLGltB+owS9z0a1jG5IvefzZ4PsHy5wi b7cSDUxsXOKXJ4bv8LJaIsky1cdJ8Ai71+/8RSDxTdOvotN5oPg== X-Received: by 2002:a25:8489:0:b0:e02:b518:7f15 with SMTP id 3f1490d57ef6-e02be1719b9mr2247914276.34.1718784863060; Wed, 19 Jun 2024 01:14:23 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFEE9E7LK6RO3INVIp8ixhLkjm2+7hNPvy6WhNyH2VUxF7p0UgwvdHX2IZZjhGIrBaXB2j59A== X-Received: by 2002:a25:8489:0:b0:e02:b518:7f15 with SMTP id 3f1490d57ef6-e02be1719b9mr2247900276.34.1718784862418; Wed, 19 Jun 2024 01:14:22 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-4449a1d67dasm18592971cf.44.2024.06.19.01.14.21 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 19 Jun 2024 01:14:21 -0700 (PDT) Date: Wed, 19 Jun 2024 10:13:46 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 1/6] conf, passt: Don't try to log to stderr after we close it Message-ID: <20240619101338.62a19245@elisabeth> In-Reply-To: References: <20240617120319.1206857-1-sbrivio@redhat.com> <20240617120319.1206857-2-sbrivio@redhat.com> <20240618080004.5fb4e355@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: SEVO7WL6UHC3THIY5SNW2JZROEQMVQHG X-Message-ID-Hash: SEVO7WL6UHC3THIY5SNW2JZROEQMVQHG X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Yalan Zhang X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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: > > =20 > > > On Mon, Jun 17, 2024 at 02:03:14PM +0200, Stefano Brivio wrote: =20 > > > > If we don't run in foreground, we close standard error as we > > > > daemonise, so it makes no sense to check if the controlling termina= l > > > > is an interactive terminal or if --force-stderr was given, to decid= e > > > > if we want to log to standard error. > > > >=20 > > > > Make --force-stderr depend on --foreground. > > > >=20 > > > > Signed-off-by: Stefano Brivio =20 > > >=20 > > > Reviewed-by: David Gibson > > > =20 > > > > --- > > > > conf.c | 3 +++ > > > > passt.c | 2 +- > > > > 2 files changed, 4 insertions(+), 1 deletion(-) > > > >=20 > > > > 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 **arg= v) > > > > =20 > > > > =09conf_ugid(runas, &uid, &gid); > > > > =20 > > > > +=09if (!c->foreground && c->force_stderr) > > > > +=09=09die("Can't log to standard error if not running in foregroun= d"); > > > > + > > > > =09if (logfile) { > > > > =09=09logfile_init(c->mode =3D=3D MODE_PASTA ? "pasta" : "passt", > > > > =09=09=09 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) > > > > =09if (isolate_prefork(&c)) > > > > =09=09die("Failed to sandbox process, exiting"); > > > > =20 > > > > -=09if (!c.force_stderr && !isatty(fileno(stderr))) > > > > +=09if (!c.foreground || (!c.force_stderr && !isatty(fileno(stderr)= ))) =20 > > >=20 > > > What's the rationale for the isatty() check in any case? =20 > >=20 > > To implement the behaviour from the man page: > >=20 > > -e, --stderr > > Log to standard error too. Default is to log to the sys= =E2=80=90 > > tem logger only, if started from an interactive terminal= , > > and to both system logger and standard error otherwise. = =20 >=20 > 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: =09if (!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. > >=20 > > Given that KubeVirt doesn't have a system logger, it was otherwise > > impossible to get messages out of passt. > >=20 > > 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. =20 >=20 > 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. --=20 Stefano