netdev - Re: [PATCH] net: phylink: Separating two unrelated definitions for improving code readability

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zz8Xve4kmHgPx-od@shell.armlinux.org.uk>
Date: Thu, 21 Nov 2024 11:21:33 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Cong Yi <yicong.srfy@...mail.com>, andrew@...n.ch, hkallweit1@...il.com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	yicong@...inos.cn
Subject: Re: [PATCH] net: phylink: Separating two unrelated definitions for
 improving code readability

On Thu, Nov 21, 2024 at 12:50:44PM +0200, Vladimir Oltean wrote:
> On Wed, Nov 20, 2024 at 05:46:14PM +0800, Cong Yi wrote:
> > Hi, Russell King:
> > 
> > Thank you for your reply!
> > Yes, as you say, there is no problem with the definitions themselves
> > being named. When I just read from Linux-5.4 to 6.6, I thought
> > that PCS_STATE_ and PHYLINK_DISABLE- were associated in some way.
> > After reading the code carefully, I found that there was no correlation。
> > In order to avoid similar confusion, I sent this patch.
> 
> For the record, I agree that tying together unrelated constants inside
> the same anonymous enum and resetting the counter is a confusing coding
> pattern, to which I don't see the benefit. Separating them and giving
> names to the enums also gives the opportunity for stronger typing, which
> was done here. I think the patch (or at least its idea) is ok.

See include/linux/ata.h, and include/linux/libata.h.

We also have many enums that either don't use the enum counter, or set
the counter to a specific value.

The typing argument is nonsense. This is a common misconception by C
programmers. You don't get any extra typechecking with enums. If you
define two enums, say fruit and colour, this produces no warning,
even with -Wall -pedantic:

enum fruit { APPLE, ORANGE };
enum colour { BLACK, WHITE };
enum fruit get_fruit(void);
enum colour test(void)
{
	return get_fruit();
}

What one gets is more compiler specific variability in the type -
some compiler architectures may use storage sufficient to store the
range of values defined in the enum (e.g. it may select char vs int
vs long) which makes laying out structs with no holes harder.

Another thing one gets is checking in switch() statmeents that all
values in the enumerated type have a "case" or "default". That's
fine where we need to ensure that all values of an enum are checked,
but if that's unnecessary, it becomes an unnecessary burden to
remember to add a - sometimes - useless default case to each switch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!