From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DC35D45B80; Sun, 20 Oct 2024 09:16:37 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 655C940268; Sun, 20 Oct 2024 09:16:37 +0200 (CEST) Received: from mail-oa1-f52.google.com (mail-oa1-f52.google.com [209.85.160.52]) by mails.dpdk.org (Postfix) with ESMTP id 2884A40156 for ; Sun, 20 Oct 2024 09:16:35 +0200 (CEST) Received: by mail-oa1-f52.google.com with SMTP id 586e51a60fabf-27d0e994ae3so1544183fac.3 for ; Sun, 20 Oct 2024 00:16:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=weka.io; s=google; t=1729408594; x=1730013394; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=O1D1m8+7A3vn77KKbUJhmr1OMzYHa33XWAtBZSqcQwk=; b=Mt/5SiDfT6lwHMSDgUYTlH3XLInbs6cPKAYMD3aJ7SdxRCPkZOmj4xWmHUgDqYolo/ Fz/yjrKi3cnb3JaS9aD0sYjALV/Qf6kWoD6i1PvAJfbdIepqH8Lt0WU0vc9OWErVdWKZ ciUSkeba7Q0cnk23a4O3UOJ05cbb077hSwFDg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729408594; x=1730013394; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=O1D1m8+7A3vn77KKbUJhmr1OMzYHa33XWAtBZSqcQwk=; b=ui1B2bRUvvxCOYeB7IF23C6tKCIFci9GNTKNA9O0YVCfaREw6kn8uAWIRYGBF3LI/P LYjor++UI6iY3dOZisBc429ZehbfHprFLAsuDFF30K3dm+FCbT2AHC2KFkrP3hPHe6LA N7QHhecq+sn2GF5r+GSc94R34NXI83lZuyNAJorCI1jI6GzdzhhZBh784kZcGs6IQesa vq8re0VgPl8f1UFSeaU29Jiiw5A9d8nrVUby5rn4M42nVmNnNcYzGDYoVYo+wJCnwWUN ka7ACbVRbDn+k4Q3DHtR2bzfyasLDOZLkRGvMExqUwX0yjjfgkYRYncp2JsTC9p69pN7 +G5g== X-Gm-Message-State: AOJu0YwYXsXkz4fE8iFxGOg31YZK2DV1x41XY0ZI7sS+GwrY92/heWrd 3agM+ZNU7XtmgFlPAyqvegObkrSgyxTeMWWeiqSuOLo3biwo5XbvF36tDYoNVwp76bJATJ/dqGO 6H5FdsOBpa6GHy4QWGXYDd4yvUxq8oO1x+PyPGfLxtRI6nbyJqaQzSsDrskBfP9k8Xe14M4cRTp 0B3+xKI6pimCtl1zPbkSTgGr/CEm14uF8/DA+1mGo9UyoDZelzzabMS2luh//4+DiWD0oTRYFa+ /nHaPViFASrkATczQ== X-Google-Smtp-Source: AGHT+IHz46vv5J2ZgIrg7Sjvoz/jbIZw4zdpQ8eVfUpNqvHKaQn8NvJ/l7Ii7WU/5vEEfKxeDPAVOjMum+af444moWc= X-Received: by 2002:a05:6870:d607:b0:288:3762:f7f5 with SMTP id 586e51a60fabf-2892c5cf565mr4622501fac.47.1729408593990; Sun, 20 Oct 2024 00:16:33 -0700 (PDT) MIME-Version: 1.0 References: <20200814173441.23086-1-stephen@networkplumber.org> <20241016202343.190653-1-stephen@networkplumber.org> In-Reply-To: <20241016202343.190653-1-stephen@networkplumber.org> From: Baruch Even Date: Sun, 20 Oct 2024 10:16:22 +0300 Message-ID: Subject: Re: [PATCH v26 00/15] Log subsystem improvements To: Stephen Hemminger Cc: dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000eabc710624e350c2" X-CLOUD-SEC-AV-Sent: true X-CLOUD-SEC-AV-Info: weka,google_mail,monitor X-Gm-Spam: 0 X-Gm-Phishy: 0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000eabc710624e350c2 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, I'm not sure about the proper way to do a code review here, feel free to educate me if there is a better way to do it. I couldn't find the actual patches text in the mail and patchwork has no interface to write comments on the line itself but I may very well have missed something. https://patches.dpdk.org/project/dpdk/patch/20241016202343.190653-13-stephe= n@networkplumber.org/ Why not provide an option to disable the syslog with --syslog=3Dnone? It will allow overriding a previous setting if one was given as well. (the same applies to the --log-journal option) https://patches.dpdk.org/project/dpdk/patch/20241016202343.190653-15-stephe= n@networkplumber.org/ You have both --log-color=3Dnever and --log-color=3Dnone, you probably need= to make up your mind, my vote would be with none for better compatibility with the syslog/journal options I made above. But I don't care that much about it. Would also be good to document about the dark mode option so people don't need to find it only in the code. Baruch On Wed, Oct 16, 2024 at 11:24=E2=80=AFPM Stephen Hemminger < stephen@networkplumber.org> wrote: > Improvements and unification of logging library. > This version works on all platforms: Linux, Windows and FreeBSD. > > This is update to rework patch set. It adds several new features > to the console log output. > > * Putting a timestamp on console output which is useful for > analyzing performance of startup codes. Timestamp is optional > and must be enabled on command line. > > * Displaying console output with colors. > It uses the standard conventions used by many other Linux commands > for colorized display. The default is to enable color if the > console output is going to a terminal. But it can be always > on or disabled by command line flag. This default was chosen > based on what dmesg(1) command does. > > Color is used by many tools (vi, iproute2, git) because it is helpful= ; > DPDK drivers and libraries print lots of not very useful messages. > And having error messages highlighted in bold face helps. > This might also get users to pay more attention to error messages. > Many bug reports have earlier messages that are lost because > there are so many info messages. > > * Add support for automatic detection of systemd journal > protocol. If running as systemd service will get enhanced > logging. > > * Use of syslog is optional and the meaning of the > --syslog flag has changed. The default is *not* to use > syslog if output is going to a terminal. > > Add myself as maintainer for log because by now have added > more than previous authors. > > v26 - rebase and change release note > - by default, do not disable color > - support dark mode with colors > - when using color format message to string then print > to avoid getting multiple threads intermixing on output. > > Stephen Hemminger (15): > maintainers: add for log library > windows: make getopt functions have const properties > windows: add os shim for localtime_r > eal: make eal_log_level_parse common > eal: do not duplicate rte_init_alert() messages > eal: change rte_exit() output to match rte_log() > log: move handling of syslog facility out of eal > eal: initialize log before everything else > log: drop syslog support, and make code common > log: add hook for printing log messages > log: add timestamp option > log: add optional support of syslog > log: add support for systemd journal > log: colorize log output > doc: add release note about log library > > MAINTAINERS | 1 + > app/test/test_eal_flags.c | 64 ++++- > doc/guides/linux_gsg/linux_eal_parameters.rst | 27 -- > doc/guides/prog_guide/log_lib.rst | 76 +++++- > doc/guides/rel_notes/release_24_11.rst | 26 ++ > lib/eal/common/eal_common_debug.c | 6 +- > lib/eal/common/eal_common_options.c | 135 ++++++---- > lib/eal/common/eal_internal_cfg.h | 1 - > lib/eal/common/eal_options.h | 7 + > lib/eal/freebsd/eal.c | 64 +---- > lib/eal/linux/eal.c | 68 +---- > lib/eal/windows/eal.c | 49 +--- > lib/eal/windows/getopt.c | 23 +- > lib/eal/windows/include/getopt.h | 8 +- > lib/eal/windows/include/rte_os_shim.h | 10 + > lib/log/log.c | 71 ++++-- > lib/log/log_color.c | 216 ++++++++++++++++ > lib/log/log_freebsd.c | 12 - > lib/log/log_internal.h | 28 +- > lib/log/log_journal.c | 200 +++++++++++++++ > lib/log/log_linux.c | 61 ----- > lib/log/log_private.h | 61 +++++ > lib/log/log_syslog.c | 88 +++++++ > lib/log/log_timestamp.c | 240 ++++++++++++++++++ > lib/log/log_windows.c | 18 -- > lib/log/meson.build | 12 +- > lib/log/version.map | 5 +- > 27 files changed, 1197 insertions(+), 380 deletions(-) > create mode 100644 lib/log/log_color.c > delete mode 100644 lib/log/log_freebsd.c > create mode 100644 lib/log/log_journal.c > delete mode 100644 lib/log/log_linux.c > create mode 100644 lib/log/log_private.h > create mode 100644 lib/log/log_syslog.c > create mode 100644 lib/log/log_timestamp.c > delete mode 100644 lib/log/log_windows.c > > -- > 2.45.2 > > --=20 Baruch Even Platform Technical Lead at WEKA E baruch@weka.io* =C2=AD*W https://www.weka.io* =C2=AD* [image: App Banner Image] --000000000000eabc710624e350c2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

I'm not sure about t= he proper way to do a code review here, feel free to educate me if there is= a better way to do it. I couldn't find the actual patches text in the = mail and patchwork has no interface to write comments on the line itself bu= t I may very well have missed something.

Why not prov= ide an option to disable the syslog with --syslog=3Dnone?
It will= allow overriding a previous setting if one was given as well.
(the same applies to the --log-journal option)

<= /div>=
You have both --log-color=3Dnever and --log-color=3Dnone, you probably= need to make up your mind, my vote would be with none for better compatibi= lity with the syslog/journal options I made above. But I don't care tha= t much about it.

Would also be good to document ab= out the dark mode option so people don't need to find it only in the co= de.

Baruch


=


On Wed, Oct 16, 2024 at 11:24=E2=80=AFPM Stephen Hemminger = <stephen@networkplumber.or= g> wrote:
Improvements and unification of logging library.
This version works on all platforms: Linux, Windows and FreeBSD.

This is update to rework patch set. It adds several new features
to the console log output.

=C2=A0 * Putting a timestamp on console output which is useful for
=C2=A0 =C2=A0 analyzing performance of startup codes. Timestamp is optional=
=C2=A0 =C2=A0 and must be enabled on command line.

=C2=A0 * Displaying console output with colors.
=C2=A0 =C2=A0 It uses the standard conventions used by many other Linux com= mands
=C2=A0 =C2=A0 for colorized display.=C2=A0 The default is to enable color i= f the
=C2=A0 =C2=A0 console output is going to a terminal. But it can be always =C2=A0 =C2=A0 on or disabled by command line flag. This default was chosen<= br> =C2=A0 =C2=A0 based on what dmesg(1) command does.

=C2=A0 =C2=A0 Color is used by many tools (vi, iproute2, git) because it is= helpful;
=C2=A0 =C2=A0 DPDK drivers and libraries print lots of not very useful mess= ages.
=C2=A0 =C2=A0 And having error messages highlighted in bold face helps.
=C2=A0 =C2=A0 This might also get users to pay more attention to error mess= ages.
=C2=A0 =C2=A0 Many bug reports have earlier messages that are lost because<= br> =C2=A0 =C2=A0 there are so many info messages.

=C2=A0 * Add support for automatic detection of systemd journal
=C2=A0 =C2=A0 protocol. If running as systemd service will get enhanced
=C2=A0 =C2=A0 logging.

=C2=A0 * Use of syslog is optional and the meaning of the
=C2=A0 =C2=A0 --syslog flag has changed. The default is *not* to use
=C2=A0 =C2=A0 syslog if output is going to a terminal.

Add myself as maintainer for log because by now have added
more than previous authors.

v26 - rebase and change release note
=C2=A0 =C2=A0 - by default, do not disable color
=C2=A0 =C2=A0 - support dark mode with colors
=C2=A0 =C2=A0 - when using color format message to string then print
=C2=A0 =C2=A0 =C2=A0 to avoid getting multiple threads intermixing on outpu= t.

Stephen Hemminger (15):
=C2=A0 maintainers: add for log library
=C2=A0 windows: make getopt functions have const properties
=C2=A0 windows: add os shim for localtime_r
=C2=A0 eal: make eal_log_level_parse common
=C2=A0 eal: do not duplicate rte_init_alert() messages
=C2=A0 eal: change rte_exit() output to match rte_log()
=C2=A0 log: move handling of syslog facility out of eal
=C2=A0 eal: initialize log before everything else
=C2=A0 log: drop syslog support, and make code common
=C2=A0 log: add hook for printing log messages
=C2=A0 log: add timestamp option
=C2=A0 log: add optional support of syslog
=C2=A0 log: add support for systemd journal
=C2=A0 log: colorize log output
=C2=A0 doc: add release note about log library

=C2=A0MAINTAINERS=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2= =A0 =C2=A01 +
=C2=A0app/test/test_eal_flags.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 64 ++++-
=C2=A0doc/guides/linux_gsg/linux_eal_parameters.rst |=C2=A0 27 --
=C2=A0doc/guides/prog_guide/log_lib.rst=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0|=C2=A0 76 +++++-
=C2=A0doc/guides/rel_notes/release_24_11.rst=C2=A0 =C2=A0 =C2=A0 =C2=A0 |= =C2=A0 26 ++
=C2=A0lib/eal/common/eal_common_debug.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0|=C2=A0 =C2=A06 +-
=C2=A0lib/eal/common/eal_common_options.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0| 135 ++++++----
=C2=A0lib/eal/common/eal_internal_cfg.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0|=C2=A0 =C2=A01 -
=C2=A0lib/eal/common/eal_options.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A07 +
=C2=A0lib/eal/freebsd/eal.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 64 +----
=C2=A0lib/eal/linux/eal.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 68 +----
=C2=A0lib/eal/windows/eal.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 49 +---
=C2=A0lib/eal/windows/getopt.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 23 +-
=C2=A0lib/eal/windows/include/getopt.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 |=C2=A0 =C2=A08 +-
=C2=A0lib/eal/windows/include/rte_os_shim.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0|=C2=A0 10 +
=C2=A0lib/log/log.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 71 ++= ++--
=C2=A0lib/log/log_color.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 216 ++++++++++++++++
=C2=A0lib/log/log_freebsd.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 12 -
=C2=A0lib/log/log_internal.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 28 +-
=C2=A0lib/log/log_journal.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 200 +++++++++++++++
=C2=A0lib/log/log_linux.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 61 -----
=C2=A0lib/log/log_private.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 61 +++++
=C2=A0lib/log/log_syslog.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 88 +++++++
=C2=A0lib/log/log_timestamp.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 240 ++++++++++++++++++
=C2=A0lib/log/log_windows.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 18 --
=C2=A0lib/log/meson.build=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 12 +-
=C2=A0lib/log/version.map=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A05 +-
=C2=A027 files changed, 1197 insertions(+), 380 deletions(-)
=C2=A0create mode 100644 lib/log/log_color.c
=C2=A0delete mode 100644 lib/log/log_freebsd.c
=C2=A0create mode 100644 lib/log/log_journal.c
=C2=A0delete mode 100644 lib/log/log_linux.c
=C2=A0create mode 100644 lib/log/log_private.h
=C2=A0create mode 100644 lib/log/log_syslog.c
=C2=A0create mode 100644 lib/log/log_timestamp.c
=C2=A0delete mode 100644 lib/log/log_windows.c

--
2.45.2



--
=
Baruch Even
Platform Technical Lead= at=C2=A0WEKA
E=C2=A0baruch@weka.io=E2=80= =85=C2=ADW=C2=A0https://www.weka.io=E2=80=85=C2=AD

= 3D"App

= =C2=A0
--000000000000eabc710624e350c2--