<div dir="ltr"><div>Hi,</div><div><br></div><div>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.</div><div><br></div><div><a href="https://patches.dpdk.org/project/dpdk/patch/20241016202343.190653-13-stephen@networkplumber.org/">https://patches.dpdk.org/project/dpdk/patch/20241016202343.190653-13-stephen@networkplumber.org/</a></div><div>Why not provide an option to disable the syslog with --syslog=none?</div><div>It will allow overriding a previous setting if one was given as well.</div><div><br></div><div>(the same applies to the --log-journal option)</div><div><br></div><div><a href="https://patches.dpdk.org/project/dpdk/patch/20241016202343.190653-15-stephen@networkplumber.org/">https://patches.dpdk.org/project/dpdk/patch/20241016202343.190653-15-stephen@networkplumber.org/</a></div><div>You have both --log-color=never and --log-color=none, 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.</div><div><br></div><div>Would also be good to document about the dark mode option so people don't need to find it only in the code.</div><div><br></div><div>Baruch<br></div><div><br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 16, 2024 at 11:24 PM Stephen Hemminger <<a href="mailto:stephen@networkplumber.org">stephen@networkplumber.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Improvements and unification of logging library.<br> This version works on all platforms: Linux, Windows and FreeBSD.<br> <br> This is update to rework patch set. It adds several new features<br> to the console log output.<br> <br> * Putting a timestamp on console output which is useful for<br> analyzing performance of startup codes. Timestamp is optional<br> and must be enabled on command line.<br> <br> * Displaying console output with colors.<br> It uses the standard conventions used by many other Linux commands<br> for colorized display. The default is to enable color if the<br> console output is going to a terminal. But it can be always<br> on or disabled by command line flag. This default was chosen<br> based on what dmesg(1) command does.<br> <br> Color is used by many tools (vi, iproute2, git) because it is helpful;<br> DPDK drivers and libraries print lots of not very useful messages.<br> And having error messages highlighted in bold face helps.<br> This might also get users to pay more attention to error messages.<br> Many bug reports have earlier messages that are lost because<br> there are so many info messages.<br> <br> * Add support for automatic detection of systemd journal<br> protocol. If running as systemd service will get enhanced<br> logging.<br> <br> * Use of syslog is optional and the meaning of the<br> --syslog flag has changed. The default is *not* to use<br> syslog if output is going to a terminal.<br> <br> Add myself as maintainer for log because by now have added<br> more than previous authors.<br> <br> v26 - rebase and change release note<br> - by default, do not disable color<br> - support dark mode with colors<br> - when using color format message to string then print<br> to avoid getting multiple threads intermixing on output.<br> <br> Stephen Hemminger (15):<br> maintainers: add for log library<br> windows: make getopt functions have const properties<br> windows: add os shim for localtime_r<br> eal: make eal_log_level_parse common<br> eal: do not duplicate rte_init_alert() messages<br> eal: change rte_exit() output to match rte_log()<br> log: move handling of syslog facility out of eal<br> eal: initialize log before everything else<br> log: drop syslog support, and make code common<br> log: add hook for printing log messages<br> log: add timestamp option<br> log: add optional support of syslog<br> log: add support for systemd journal<br> log: colorize log output<br> doc: add release note about log library<br> <br> MAINTAINERS | 1 +<br> app/test/test_eal_flags.c | 64 ++++-<br> doc/guides/linux_gsg/linux_eal_parameters.rst | 27 --<br> doc/guides/prog_guide/log_lib.rst | 76 +++++-<br> doc/guides/rel_notes/release_24_11.rst | 26 ++<br> lib/eal/common/eal_common_debug.c | 6 +-<br> lib/eal/common/eal_common_options.c | 135 ++++++----<br> lib/eal/common/eal_internal_cfg.h | 1 -<br> lib/eal/common/eal_options.h | 7 +<br> lib/eal/freebsd/eal.c | 64 +----<br> lib/eal/linux/eal.c | 68 +----<br> lib/eal/windows/eal.c | 49 +---<br> lib/eal/windows/getopt.c | 23 +-<br> lib/eal/windows/include/getopt.h | 8 +-<br> lib/eal/windows/include/rte_os_shim.h | 10 +<br> lib/log/log.c | 71 ++++--<br> lib/log/log_color.c | 216 ++++++++++++++++<br> lib/log/log_freebsd.c | 12 -<br> lib/log/log_internal.h | 28 +-<br> lib/log/log_journal.c | 200 +++++++++++++++<br> lib/log/log_linux.c | 61 -----<br> lib/log/log_private.h | 61 +++++<br> lib/log/log_syslog.c | 88 +++++++<br> lib/log/log_timestamp.c | 240 ++++++++++++++++++<br> lib/log/log_windows.c | 18 --<br> lib/log/meson.build | 12 +-<br> lib/log/version.map | 5 +-<br> 27 files changed, 1197 insertions(+), 380 deletions(-)<br> create mode 100644 lib/log/log_color.c<br> delete mode 100644 lib/log/log_freebsd.c<br> create mode 100644 lib/log/log_journal.c<br> delete mode 100644 lib/log/log_linux.c<br> create mode 100644 lib/log/log_private.h<br> create mode 100644 lib/log/log_syslog.c<br> create mode 100644 lib/log/log_timestamp.c<br> delete mode 100644 lib/log/log_windows.c<br> <br> -- <br> 2.45.2<br> <br> </blockquote></div><br clear="all"><br><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><table style="direction:ltr;border-collapse:collapse"><tbody><tr><td style="font-size:0px;height:24px;line-height:0"></td></tr><tr><td><table cellpadding="0" cellspacing="0" border="0" style="width:100%" width="100%"><tbody><tr><td><div><div><table border="0" cellspacing="0" cellpadding="0" width="510" style="width:510px"><tbody><tr><td valign="top" style="vertical-align:top;padding-right:12px;width:120px" width="120"><table border="0" cellspacing="0" cellpadding="0"><tbody><tr><td><img src="https://cdn.gifo.wisestamp.com/im/sh/aHR0cHM6Ly9kNGQ4eGQyMGVyOGxnLmNsb3VkZnJvbnQubmV0LzBjOTUwMTdkLTAzZjMtNGIwZC1iZDM0LTVkMzkzOGEzNjM4ZS9qNXJvQXhodUVRMGRqVmlZcmhzQ2p6VDVmaWhDZTRLUi5wbmcjbG9nbw==/rounded.png" width="120" height="92" style="width: 120px; height: 92px; border-radius: 4px;"></td></tr></tbody></table></td><td><table border="0" cellspacing="0" cellpadding="0" width="100%" style="width:100%"><tbody><tr><td><table border="0" cellpadding="0" cellspacing="0" width="100%" style="color:rgb(78,75,76);padding-left:2px;font-weight:normal;width:100%;padding-bottom:5px;border-bottom:5px solid rgb(212,212,212);font-size:16px"><tbody><tr><td><span style="color:rgb(119,61,190);line-height:200%;font-weight:bold;font-family:Arial">Baruch Even</span><br><span style="font-family:Arial;color:rgb(51,51,51);line-height:200%">Platform Technical Lead</span> <span style="font-family:Arial;line-height:200%;color:rgb(51,51,51)">at </span><span style="color:rgb(119,61,190);line-height:200%;font-family:Arial">WEKA</span></td></tr></tbody></table></td></tr><tr><td><table border="0" cellpadding="0" cellspacing="0" align="left" width="100%" style="width:100%"><tbody><tr><td style="font-size:16px"><span style="color:rgb(119,61,190);line-height:150%;font-weight:bold;font-family:Arial">E </span><a href="mailto:baruch@weka.io" style="text-decoration:none;line-height:150%;color:rgb(78,75,76);font-family:Arial" target="_blank">baruch@weka.io</a><i style="color:rgb(255,255,255)"> </i><span style="color:rgb(119,61,190);line-height:150%;font-weight:bold;font-family:Arial">W </span><a href="https://www.weka.io" style="text-decoration:none;line-height:150%;color:rgb(78,75,76);font-family:Arial" target="_blank">https://www.weka.io</a><i style="color:rgb(255,255,255)"> </i></td></tr></tbody></table></td></tr></tbody></table></td></tr></tbody></table></div></div></td></tr><tr><td style="line-height:1%;padding-top:8px;font-size:1px"></td></tr><tr><td><table cellpadding="0" cellspacing="0" style="line-height:normal;width:100%" width="100%"><tbody><tr><td style="text-align:left"><p style="margin:1px"><a href="https://www.weka.io/trends-in-AI-emsig?utm_campaign=signature&utm_source=WiseStamp&utm_medium=email" rel="nofollow noreferrer" target="_blank"><img src="https://d36urhup7zbd7q.cloudfront.net/u/MnDyROw8y77/324e8ddb-ebe2-4864-a177-a690bc3babbc__760x286__.png" style="width: 299px; height: 112px;" width="299" height="112" alt="App Banner Image"></a></p></td></tr></tbody></table></td></tr><tr><td style="line-height:1%;padding-top:8px;font-size:1px"></td></tr></tbody></table></td></tr><tr><td style="font-family:"ws-id 9qgYxGOb9J9R";font-size:0.01px;line-height:0"> </td></tr></tbody></table></div></div>