netdev - Re: [iproute PATCH] lib/color: introduce freely configurable color strings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201012080454.6b57ccfe@hermes.local>
Date:   Mon, 12 Oct 2020 08:04:54 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Jan Engelhardt <jengelh@...i.de>
Cc:     netdev@...r.kernel.org
Subject: Re: [iproute PATCH] lib/color: introduce freely configurable color
 strings

On Mon, 12 Oct 2020 16:50:21 +0200
Jan Engelhardt <jengelh@...i.de> wrote:

> Implement fine-grained control over color codes for iproute, very
> similar to the GCC_COLORS environment variable.
> 
> Signed-off-by: Jan Engelhardt <jengelh@...i.de>
> ---
>  lib/color.c   | 127 ++++++++++++++++++++++++--------------------------
>  man/man8/ip.8 |  25 ++++++++--
>  2 files changed, 81 insertions(+), 71 deletions(-)

I like the idea a lot.

But there are some style issues.
Checkpatch sees:

WARNING: Missing a blank line after declarations
#165: FILE: lib/color.c:46:
+	const char *s = getenv("COLORTERM"), *s2 = getenv("COLORFGBG");
+	color_is_enabled = (s != NULL && strtoul(s, NULL, 0) != 0) ||

ERROR: code indent should use tabs where possible
#166: FILE: lib/color.c:47:
+^I                   (s2 != NULL && *s2 != '\0');$

ERROR: do not use assignment in if condition
#188: FILE: lib/color.c:99:
+	if (p && (p = strrchr(p, ';')) != NULL && (p[1] == '7' && p[2] == '\0'))

WARNING: Missing a blank line after declarations
#197: FILE: lib/color.c:108:
+		const char *code = NULL;
+		for (size_t j = 0; j < ARRAY_SIZE(color_codes); ++j) {

WARNING: Missing a blank line after declarations
#207: FILE: lib/color.c:118:
+		const char *next = strchr(p, ':');
+		if (next == NULL)

WARNING: Missing a blank line after declarations
#228: FILE: lib/color.c:144:
+	const struct color_code *k = &color_codes[attr];
+	if (k->code != NULL && *k->code != '\0')


Also, please don't mix declarations in code like:

+		for (size_t j = 0; j < ARRAY_SIZE(color_codes); ++j) {

Iproute2 tries to stick to the kernel coding style, and loop variables
like this are allowed in that standard.