netdev - Re: [Intel-wired-lan] [PATCH net-next] iavf: remove double underscores from states names

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

Message-ID: <21b89328-8c74-596b-3cfe-e71affd193a8@intel.com>
Date:   Thu, 20 Apr 2023 16:33:54 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Ahmed Zaki <ahmed.zaki@...el.com>
CC:     <intel-wired-lan@...ts.osuosl.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH net-next] iavf: remove double
 underscores from states names

From: Ahmed Zaki <ahmed.zaki@...el.com>
Date: Wed, 19 Apr 2023 17:17:51 -0600

+ netdev

Please add netdev ML next time you send networking stuff, not only Jakub :D

> No need to prepend some states names with "__", especially that others do
> not have these underscores.

What's the value of this change, apart from 400 locs diffstat? I don't
say having prefixes for general-purpose definitions is correct, but...
Dunno. Would be nice to have as a part of some series with assorted
improvements, but not standalone. Or it's just me =\

> 
> Link: https://lore.kernel.org/netdev/20230406130415.1aa1d1cd@kernel.org
> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@...el.com>

[...]

> @@ -468,36 +468,36 @@ extern char iavf_driver_name[];
>  static inline const char *iavf_state_str(enum iavf_state_t state)
>  {
>  	switch (state) {
> -	case __IAVF_STARTUP:
> -		return "__IAVF_STARTUP";
> -	case __IAVF_REMOVE:
> -		return "__IAVF_REMOVE";
> -	case __IAVF_INIT_VERSION_CHECK:
> -		return "__IAVF_INIT_VERSION_CHECK";
> -	case __IAVF_INIT_GET_RESOURCES:
> -		return "__IAVF_INIT_GET_RESOURCES";
> -	case __IAVF_INIT_EXTENDED_CAPS:
> -		return "__IAVF_INIT_EXTENDED_CAPS";
> -	case __IAVF_INIT_CONFIG_ADAPTER:
> -		return "__IAVF_INIT_CONFIG_ADAPTER";
> -	case __IAVF_INIT_SW:
> -		return "__IAVF_INIT_SW";
> -	case __IAVF_INIT_FAILED:
> -		return "__IAVF_INIT_FAILED";
> -	case __IAVF_RESETTING:
> -		return "__IAVF_RESETTING";
> -	case __IAVF_COMM_FAILED:
> -		return "__IAVF_COMM_FAILED";
> -	case __IAVF_DOWN:
> -		return "__IAVF_DOWN";
> -	case __IAVF_DOWN_PENDING:
> -		return "__IAVF_DOWN_PENDING";
> -	case __IAVF_TESTING:
> -		return "__IAVF_TESTING";
> -	case __IAVF_RUNNING:
> -		return "__IAVF_RUNNING";
> +	case IAVF_STARTUP:
> +		return "IAVF_STARTUP";
> +	case IAVF_REMOVE:
> +		return "IAVF_REMOVE";
> +	case IAVF_INIT_VERSION_CHECK:
> +		return "IAVF_INIT_VERSION_CHECK";
> +	case IAVF_INIT_GET_RESOURCES:
> +		return "IAVF_INIT_GET_RESOURCES";
> +	case IAVF_INIT_EXTENDED_CAPS:
> +		return "IAVF_INIT_EXTENDED_CAPS";
> +	case IAVF_INIT_CONFIG_ADAPTER:
> +		return "IAVF_INIT_CONFIG_ADAPTER";
> +	case IAVF_INIT_SW:
> +		return "IAVF_INIT_SW";
> +	case IAVF_INIT_FAILED:
> +		return "IAVF_INIT_FAILED";
> +	case IAVF_RESETTING:
> +		return "IAVF_RESETTING";
> +	case IAVF_COMM_FAILED:
> +		return "IAVF_COMM_FAILED";
> +	case IAVF_DOWN:
> +		return "IAVF_DOWN";
> +	case IAVF_DOWN_PENDING:
> +		return "IAVF_DOWN_PENDING";
> +	case IAVF_TESTING:
> +		return "IAVF_TESTING";
> +	case IAVF_RUNNING:
> +		return "IAVF_RUNNING";
>  	default:
> -		return "__IAVF_UNKNOWN_STATE";
> +		return "IAVF_UNKNOWN_STATE";
>  	}
>  }

If you want to do something really useful, pls replace that non-sense
with O(1) array lookup:

iavf_something.c:

const char * const iavf_states[] = {
#define __IAVF_STATE_STR(s)	[IAVF_##s]	= "IAVF_" #s
	__IAVF_STATE_STR(STARTUP),
	__IAVF_STATE_STR(REMOVE),
...
	__IAVF_STATE_STR(RUNNING),
#undef __IAVF_STATE_STR
	"IAVF_UNKNOWN_STATE",
}

iavf.h:

extern const char * const iavf_states[];

static inline const char *iavf_state_str(enum iavf_state_t state)
{
	if (unlikely(state > IAVF_STATE_RUNNING))
		state = IAVF_STATE_RUNNING + 1;

	// ^ or better just add `IAVF_UNKNOWN_STATE` to the enum
	// to not open-code `RUNNING + 1`

	return iavf_states[state];
}

You can even just copy'n'paste that and make a patch, I think it would
work :D
And attach bloat-o-meter output to the commit message, I'd like to watch
the result :3

[...]

Thanks,
Olek