From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 041065A0050 for ; Thu, 20 Jun 2024 02:49:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718844566; bh=+eJsb42CPoIBCJOQieGsuYsW1BIeZwOuIhNZRYnyGv4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LlFG/F+0l3ZXlg2MRXqTVnQmMhMz4oStiM/jHMPCmzzcYI4PGCotmjrmroWgRsNzb gvB0gvZ/4OfUQqmpo1E6lMfKWjwdHpi3LMuwe5i9+5lgLlrBr8gO4US56NJqr1dtbS d+Okc+v5mTXwD5ybLgskVaoq96CvYPz2VBYwz4vMo0Jrj4rzRc6iR5Qbk7atT2T1Ro Ebi2zsObY1IzL0SNUFL82jo0413YlV72d6z3RA2MlcaxugJXovX4fx004N3hU0p9J4 EteRGZzQEkmCoP48pce1GGkqv4E971R2LGie2EXqI5Jp5dIidqw4PsOchVfjT3mVJ9 fdNr/TU2lHvJw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W4MMB4kgmz4wyf; Thu, 20 Jun 2024 10:49:26 +1000 (AEST) Date: Thu, 20 Jun 2024 10:31:31 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 2/8] conf, passt: Make --stderr do nothing, and deprecate it Message-ID: References: <20240619194028.2913930-1-sbrivio@redhat.com> <20240619194028.2913930-3-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="zQA7Tu76m/lsOc6Q" Content-Disposition: inline In-Reply-To: <20240619194028.2913930-3-sbrivio@redhat.com> Message-ID-Hash: ORNTSCNCOV5FEXUUMAOVJIX64I66DF3E X-Message-ID-Hash: ORNTSCNCOV5FEXUUMAOVJIX64I66DF3E X-MailFrom: dgibson@gandalf.ozlabs.org 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: --zQA7Tu76m/lsOc6Q Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 19, 2024 at 09:40:22PM +0200, Stefano Brivio wrote: > The original behaviour of printing messages to standard error by > default when running from a non-interactive terminal was introduced > because the first KubeVirt integration draft used to start passt in > foreground and get messages via standard error. >=20 > For development purposes, the system logger was more convenient at > that point, and passt was running from interactive terminals only if > not started by the KubeVirt integration. >=20 > This behaviour was introduced by 84a62b79a2bc ("passt: Also log to > stderr, don't fork to background if not interactive"). >=20 > Later, I added command-line options in 1e49d194d017 ("passt, pasta: > Introduce command-line options and port re-mapping") and accidentally > reversed this condition, which wasn't a problem as --stderr could > force printing to standard error anyway (and it was used by KubeVirt). >=20 > Nowadays, the KubeVirt integration uses a log file (requested via > libvirt configuration), and the same applies for Podman if one > actually needs to look at runtime logs. There are no use cases left, > as far as I know, where passt runs in foreground in non-interactive > terminals. >=20 > Seize the chance to reintroduce some sanity here. If we fork to > background, standard error is closed, so --stderr is useless in that > case. >=20 > If we run in foreground, there's no harm in printing messages to > standard error, and that accidentally became the default behaviour > anyway, so --stderr is not needed in that case. >=20 > It would be needed for non-interactive terminals, but there are no > use cases, and if there were, let's log to standard error anyway: > the user can always redirect standard error to /dev/null if needed. >=20 > Before we're up and running, we need to print to standard error anyway > if something happens, otherwise we can't report failure to start in > any kind of usage, stand-alone or in integrations. >=20 > So, make --stderr do nothing, and deprecate it. >=20 > While at it, drop a left-over comment about --foreground being the > default only for interactive terminals, because it's not the case > anymore. >=20 > Signed-off-by: Stefano Brivio Reviewed-by: David Gibson > --- > conf.c | 18 ++---------------- > passt.1 | 9 +++++---- > passt.c | 2 +- > passt.h | 2 -- > 4 files changed, 8 insertions(+), 23 deletions(-) >=20 > diff --git a/conf.c b/conf.c > index dbdbb62..9f869f5 100644 > --- a/conf.c > +++ b/conf.c > @@ -730,9 +730,7 @@ static void usage(const char *name, FILE *f, int stat= us) > " --trace Be extra verbose, implies --debug\n" > " -q, --quiet Don't print informational messages\n" > " -f, --foreground Don't run in background\n" > - " default: run in background if started from a TTY\n" > - " -e, --stderr Log to stderr too\n" > - " default: log to system logger only if started from a TTY\n" > + " default: run in background\n" > " -l, --log-file PATH Log (only) to given file\n" > " --log-size BYTES Maximum size of log file\n" > " default: 1 MiB\n" > @@ -1405,18 +1403,9 @@ void conf(struct ctx *c, int argc, char **argv) > c->debug =3D 1; > break; > case 'e': > - if (logfile) > - die("Can't log to both file and stderr"); > - > - if (c->force_stderr) > - die("Multiple --stderr options given"); > - > - c->force_stderr =3D 1; > + warn("--stderr will be dropped soon"); > break; > case 'l': > - if (c->force_stderr) > - die("Can't log to both stderr and file"); > - > if (logfile) > die("Multiple --log-file options given"); > =20 > @@ -1693,9 +1682,6 @@ void conf(struct ctx *c, int argc, char **argv) > =20 > conf_ugid(runas, &uid, &gid); > =20 > - if (!c->foreground && c->force_stderr) > - die("Can't log to standard error if not running in foreground"); > - > if (logfile) { > logfile_init(c->mode =3D=3D MODE_PASTA ? "pasta" : "passt", > logfile, logsize); > diff --git a/passt.1 b/passt.1 > index 7676fe3..6c8f932 100644 > --- a/passt.1 > +++ b/passt.1 > @@ -93,13 +93,14 @@ Default is to fork into background. > =20 > .TP > .BR \-e ", " \-\-stderr > -Log to standard error too. > -Default is to log to the system logger only, if started from an interact= ive > -terminal, and to both system logger and standard error otherwise. > +This option has no effect, and is maintained for compatibility purposes = only. > + > +Note that this configuration option is \fBdeprecated\fR and will be remo= ved in a > +future version. > =20 > .TP > .BR \-l ", " \-\-log-file " " \fIPATH\fR > -Log to file \fIPATH\fR, not to standard error, and not to the system log= ger. > +Log to file \fIPATH\fR, and not to the system logger. > =20 > .TP > .BR \-\-log-size " " \fISIZE\fR > diff --git a/passt.c b/passt.c > index aa9648a..19ecd68 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"); > =20 > - if (!c.foreground || (!c.force_stderr && !isatty(fileno(stderr)))) > + if (!c.foreground) > __openlog(log_name, 0, LOG_DAEMON); > =20 > if (!c.foreground) > diff --git a/passt.h b/passt.h > index 46d073a..21cf4c1 100644 > --- a/passt.h > +++ b/passt.h > @@ -180,7 +180,6 @@ struct ip6_ctx { > * @trace: Enable tracing (extra debug) mode > * @quiet: Don't print informational messages > * @foreground: Run in foreground, don't log to stderr by default > - * @force_stderr: Force logging to stderr > * @nofile: Maximum number of open files (ulimit -n) > * @sock_path: Path for UNIX domain socket > * @pcap: Path for packet capture file > @@ -231,7 +230,6 @@ struct ctx { > int trace; > int quiet; > int foreground; > - int force_stderr; > int nofile; > char sock_path[UNIX_PATH_MAX]; > char pcap[PATH_MAX]; --=20 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 --zQA7Tu76m/lsOc6Q Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZzeGMACgkQzQJF27ox 2GeKgg//dpNfhYwWqqaWKDVRUaIluQ6iwOJdxgOGxAHu/bpXIDHOjQSJg1LdXuFs JPv/vE5vzYSrZjjHOnAF8Gqn9BZMKeNQRHzBOhysULKzlmgWjIXq6m/57d9CH3lZ PZwSE3O9z6fu6J3IF5a2EDdgbOSyrbSd6gXpOVAKELs6pnwC2DtUVp3oYlbqvc/6 eHBQYKD4VaUQSUFJxgX5azbG7NVxxnKjiayrUQxQd1SZ/2sw+2CB5Cs3IXEacc+b BewL22PnZ/1DpN8WfJMTKAW37rNfYOF9Fy7GESuFyAxisq+CyMV4G/A9P40/HMYa pRphc7e1YEAr1A4F4ZCWVr9ldH94AZGIrWeYoxYPtYqvaj5hfAViGEjyUipO5BqT EIZZewUlGbU6RrIf/ixv3yB9F4uI9vwHuyrb5ETntRfoKjOVwUPIPzVghghT8ZZR 25fqIrk2BRZO9TGhfL9R/+T3pggEttIdDz/hA5qyN29JMZlX90tK6ncgJB/lLL7R PAovnUpDDekJ9Q7XClzchPrG2Mo/JVcxEHem+CXLnyq74FyGgpGt+jGcg12FUFnJ Z6D6AHBs3A0lGPvs9w9VkhfCt16+rIV0iQTbk0ZH74GEsrZG2P29vlKSHlFJ1Tb5 GQQjEiBA0ZLR29kiXq8bi30SFMgNb5Nngv+jtZW/RQsdlnIKFbY= =zEY2 -----END PGP SIGNATURE----- --zQA7Tu76m/lsOc6Q--