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 9E9445A004F for ; Thu, 25 Jul 2024 05:47:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1721879255; bh=ah/aZJLUcYHVmlncXVhiHN3RNGrAXe3gF8Oql10HGj4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=OFks3zDwJr3vkdyW+wKNnOA47lMNaJnuc4mostumcWWtj/tiaShGGDwjW1tOZcRpb TDvR6UTkcKO7qdeSnmPMt1bIBtvRjEowczo4OD8qavInaA6eVhFSYMzTyDGdV7A0m4 5dQ0LGaAalvefdXSZWUQEKFM/dOlsNtnmZP1SPz7g/1y/FjuORHI4Hx8t3FMmu8+hh bHhGUdPWcZ52Pa6X5uXyaEpprUapYso1w8Ns7V/xsz8J+8O/COT3CbbfBlImXtlIjb PBxLismx2sO+UuETGFemcHVJVf79nFILmXCPxp82LbgKQSIKdesP7HEzATligh3lWo OyPt3n240Wj3g== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4WTxfb4Lqdz4wbr; Thu, 25 Jul 2024 13:47:35 +1000 (AEST) Date: Thu, 25 Jul 2024 13:26:47 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 03/11] log: Drop newlines in the middle of the perror()-like messages Message-ID: References: <20240724215021.3366863-1-sbrivio@redhat.com> <20240724215021.3366863-4-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Ju32eGwMMQB5Hz+K" Content-Disposition: inline In-Reply-To: <20240724215021.3366863-4-sbrivio@redhat.com> Message-ID-Hash: 2QKZPXIVWJVMLV2Q7KJDNVVUIXRBWPHI X-Message-ID-Hash: 2QKZPXIVWJVMLV2Q7KJDNVVUIXRBWPHI 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 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: --Ju32eGwMMQB5Hz+K Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 24, 2024 at 11:50:09PM +0200, Stefano Brivio wrote: > Calling vlogmsg() twice from logmsg_perror() results in this beauty: >=20 > $ ./pasta -i foo > Invalid interface name foo > : No such device >=20 > because the first part of the message, corresponding to the first > call, doesn't end with a newline, and vlogmsg() adds it. >=20 > Given that we can't easily append an argument (error description) to > a variadic list, add a 'newline' parameter to all the functions that > currently add a newline if missing, and disable that on the first call > to vlogmsg() from logmsg_perror(). Not very pretty but I can't think > of any solution that's less messy than this. >=20 > Signed-off-by: Stefano Brivio I think my personal inclination would be to rename all the lowest-level functions slightly and remove the newline adding logic unconditionally. The create wrappers under the old name which add the "\n". I think that can be done in an easy macro, since the "\n" can be constant string appended to the format string. Just the special paths that need to suppress the newline would call the low level "no newline" variants. But, I don't actually care that much so Reviewed-by: David Gibson > --- > flow.c | 2 +- > log.c | 34 ++++++++++++++++++++++------------ > log.h | 18 +++++++++--------- > 3 files changed, 32 insertions(+), 22 deletions(-) >=20 > diff --git a/flow.c b/flow.c > index d7d548d..bd5fa2b 100644 > --- a/flow.c > +++ b/flow.c > @@ -279,7 +279,7 @@ void flow_log_(const struct flow_common *f, int pri, = const char *fmt, ...) > else > type_or_state =3D FLOW_TYPE(f); > =20 > - logmsg(pri, "Flow %u (%s): %s", flow_idx(f), type_or_state, msg); > + logmsg(true, pri, "Flow %u (%s): %s", flow_idx(f), type_or_state, msg); > } > =20 > /** > diff --git a/log.c b/log.c > index 9ed6bc1..54483e7 100644 > --- a/log.c > +++ b/log.c > @@ -46,7 +46,14 @@ int log_trace; /* --trace mode enabled */ > bool log_conf_parsed; /* Logging options already parsed */ > bool log_runtime; /* Daemonised, or ready in foreground */ > =20 > -void vlogmsg(int pri, const char *format, va_list ap) > +/** > + * vlogmsg() - Print or send messages to log or output files as configur= ed > + * @newline: Append newline at the end of the message, if missing > + * @pri: Facility and level map, same as priority for vsyslog() > + * @format: Message > + * @ap: Variable argument list > + */ > +void vlogmsg(bool newline, int pri, const char *format, va_list ap) > { > bool debug_print =3D (log_mask & LOG_MASK(LOG_DEBUG)) && log_file =3D= =3D -1; > struct timespec tp; > @@ -63,9 +70,9 @@ void vlogmsg(int pri, const char *format, va_list ap) > =20 > va_copy(ap2, ap); /* Don't clobber ap, we need it again */ > if (log_file !=3D -1) > - logfile_write(pri, format, ap2); > + logfile_write(newline, pri, format, ap2); > else if (!(log_mask & LOG_MASK(LOG_DEBUG))) > - passt_vsyslog(pri, format, ap2); > + passt_vsyslog(newline, pri, format, ap2); > =20 > va_end(ap2); > } > @@ -73,22 +80,23 @@ void vlogmsg(int pri, const char *format, va_list ap) > if (debug_print || !log_conf_parsed || > (!log_runtime && (log_mask & LOG_MASK(LOG_PRI(pri))))) { > (void)vfprintf(stderr, format, ap); > - if (format[strlen(format)] !=3D '\n') > + if (newline && format[strlen(format)] !=3D '\n') > fprintf(stderr, "\n"); > } > } > =20 > /** > * logmsg() - vlogmsg() wrapper for variable argument lists > + * @newline: Append newline at the end of the message, if missing > * @pri: Facility and level map, same as priority for vsyslog() > * @format: Message > */ > -void logmsg(int pri, const char *format, ...) > +void logmsg(bool newline, int pri, const char *format, ...) > { > va_list ap; > =20 > va_start(ap, format); > - vlogmsg(pri, format, ap); > + vlogmsg(newline, pri, format, ap); > va_end(ap); > } > =20 > @@ -103,10 +111,10 @@ void logmsg_perror(int pri, const char *format, ...) > va_list ap; > =20 > va_start(ap, format); > - vlogmsg(pri, format, ap); > + vlogmsg(false, pri, format, ap); > va_end(ap); > =20 > - logmsg(pri, ": %s", strerror(errno_copy)); > + logmsg(true, pri, ": %s", strerror(errno_copy)); > } > =20 > /* Prefixes for log file messages, indexed by priority */ > @@ -174,11 +182,12 @@ void __setlogmask(int mask) > =20 > /** > * passt_vsyslog() - vsyslog() implementation not using heap memory > + * @newline: Append newline at the end of the message, if missing > * @pri: Facility and level map, same as priority for vsyslog() > * @format: Same as vsyslog() format > * @ap: Same as vsyslog() ap > */ > -void passt_vsyslog(int pri, const char *format, va_list ap) > +void passt_vsyslog(bool newline, int pri, const char *format, va_list ap) > { > char buf[BUFSIZ]; > int n; > @@ -188,7 +197,7 @@ void passt_vsyslog(int pri, const char *format, va_li= st ap) > =20 > n +=3D vsnprintf(buf + n, BUFSIZ - n, format, ap); > =20 > - if (format[strlen(format)] !=3D '\n') > + if (newline && format[strlen(format)] !=3D '\n') > n +=3D snprintf(buf + n, BUFSIZ - n, "\n"); > =20 > if (log_sock >=3D 0 && send(log_sock, buf, n, 0) !=3D n && !log_runtime) > @@ -360,11 +369,12 @@ static int logfile_rotate(int fd, const struct time= spec *now) > =20 > /** > * logfile_write() - Write entry to log file, trigger rotation if full > + * @newline: Append newline at the end of the message, if missing > * @pri: Facility and level map, same as priority for vsyslog() > * @format: Same as vsyslog() format > * @ap: Same as vsyslog() ap > */ > -void logfile_write(int pri, const char *format, va_list ap) > +void logfile_write(bool newline, int pri, const char *format, va_list ap) > { > struct timespec now; > char buf[BUFSIZ]; > @@ -380,7 +390,7 @@ void logfile_write(int pri, const char *format, va_li= st ap) > =20 > n +=3D vsnprintf(buf + n, BUFSIZ - n, format, ap); > =20 > - if (format[strlen(format)] !=3D '\n') > + if (newline && format[strlen(format)] !=3D '\n') > n +=3D snprintf(buf + n, BUFSIZ - n, "\n"); > =20 > if ((log_written + n >=3D log_size) && logfile_rotate(log_file, &now)) > diff --git a/log.h b/log.h > index 6f34e8d..51ddafa 100644 > --- a/log.h > +++ b/log.h > @@ -13,16 +13,16 @@ > #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */ > #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE)) > =20 > -void vlogmsg(int pri, const char *format, va_list ap); > -void logmsg(int pri, const char *format, ...) > - __attribute__((format(printf, 2, 3))); > +void vlogmsg(bool newline, int pri, const char *format, va_list ap); > +void logmsg(bool newline, int pri, const char *format, ...) > + __attribute__((format(printf, 3, 4))); > void logmsg_perror(int pri, const char *format, ...) > __attribute__((format(printf, 2, 3))); > =20 > -#define err(...) logmsg( LOG_ERR, __VA_ARGS__) > -#define warn(...) logmsg( LOG_WARNING, __VA_ARGS__) > -#define info(...) logmsg( LOG_INFO, __VA_ARGS__) > -#define debug(...) logmsg( LOG_DEBUG, __VA_ARGS__) > +#define err(...) logmsg(true, LOG_ERR, __VA_ARGS__) > +#define warn(...) logmsg(true, LOG_WARNING, __VA_ARGS__) > +#define info(...) logmsg(true, LOG_INFO, __VA_ARGS__) > +#define debug(...) logmsg(true, LOG_DEBUG, __VA_ARGS__) > =20 > #define err_perror(...) logmsg_perror( LOG_ERR, __VA_ARGS__) > #define warn_perror(...) logmsg_perror( LOG_WARNING, __VA_ARGS__) > @@ -54,8 +54,8 @@ void trace_init(int enable); > =20 > void __openlog(const char *ident, int option, int facility); > void logfile_init(const char *name, const char *path, size_t size); > -void passt_vsyslog(int pri, const char *format, va_list ap); > -void logfile_write(int pri, const char *format, va_list ap); > +void passt_vsyslog(bool newline, int pri, const char *format, va_list ap= ); > +void logfile_write(bool newline, int pri, const char *format, va_list ap= ); > void __setlogmask(int mask); > =20 > #endif /* LOG_H */ --=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 --Ju32eGwMMQB5Hz+K Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmahxfYACgkQzQJF27ox 2Gc8+A/+O8CeDgVXZZHhHsbnW+LOyvMYuYH5JHvJSlAq8O3SPKou441/4vFu+snw mSs8iDC/Iaj4QTbo4Ue446eQleci8xRGxtnakH3NrFoaj9XPbxUNYqjV6bBsRx7s TspztDWew/eMr02dZ368HVxR7ZITpJIZ4M6G6mxKjGs1Z8o1PEU0NLSaX/jrPC7l ajkFLcReZmLT65HYHSXedSaw3YJ3xsIPadz1DFZ0tGRpg0j8pioL090duI+fkxzN BfR6zSzmiHeXZN+5SFVKumBn8vL0qbXyycazVeKAJ9bnrRuzmgI5Gg2gu7mLjqhB sSUdur4lgHfsDNTJSKlqnQgYJLOwDDanoD7qTEzVspVs/yh41NmljT9NnXGNiPoQ kPE6R8oLRV9rj5Gc7kTWEnLLC3vS5Fgzluhw+7GooQRz4QAkLIh65D0TMsPMoAXS gBx06RcHZw8QgCiWELLXpHfL80Eu9fflSb2c8ieTJY6+n4Tl9XoILN33rQS6j/Di obkdZ5luarUZ3Pk/CxaX7FvaRTqkO0wmgrw5jN++9rujsZ5pJb+csMjdAms4dXtQ vXQ2rRTvigHer/BVAxM6KTyzraZXd3BXOFXy6ngMwWuk30MpjTLrzN+WUxPvGZFA BA0UYlYQimcweoyKXp6kl1mTa7Y80qdA2nz4YoM6CikuZbcDM1Y= =5aVA -----END PGP SIGNATURE----- --Ju32eGwMMQB5Hz+K--