netdev - Re: [PATCH v6 1/3] netfilter: x_tables: Merge xt_*.h and ipt

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: <8f20c793-7985-72b2-6420-fd2fd27fe69c@blackhole.kfki.hu>
Date: Mon, 6 Jan 2025 09:19:57 +0100 (CET)
From: Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>
To: Benjamin Szőke <egyszeregy@...email.hu>
cc: Florian Westphal <fw@...len.de>, Pablo Neira Ayuso <pablo@...filter.org>, 
    lorenzo@...nel.org, daniel@...earbox.net, leitao@...ian.org, 
    amiculas@...co.com, kadlec@...filter.org, 
    David Miller <davem@...emloft.net>, dsahern@...nel.org, 
    edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, 
    netfilter-devel@...r.kernel.org, coreteam@...filter.org, 
    linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v6 1/3] netfilter: x_tables: Merge xt_*.h and ipt_*.h
 files which has same name.

On Mon, 6 Jan 2025, egyszeregy@...email.hu wrote:

> From: Benjamin Szőke <egyszeregy@...email.hu>
>
> Merge xt_*.h, ipt_*.h and ip6t_*.h header files, which has
> same upper and lower case name format.
>
> Add #pragma message about recommended to use
> header files with lower case format in the future.
>
> Signed-off-by: Benjamin Szőke <egyszeregy@...email.hu>
> ---
> include/uapi/linux/netfilter/xt_CONNMARK.h  |  8 +++---
> include/uapi/linux/netfilter/xt_DSCP.h      | 22 ++--------------
> include/uapi/linux/netfilter/xt_MARK.h      |  8 +++---
> include/uapi/linux/netfilter/xt_RATEEST.h   | 12 ++-------
> include/uapi/linux/netfilter/xt_TCPMSS.h    | 14 ++++------
> include/uapi/linux/netfilter/xt_connmark.h  |  7 +++--
> include/uapi/linux/netfilter/xt_dscp.h      | 20 +++++++++++---
> include/uapi/linux/netfilter/xt_mark.h      |  6 ++---
> include/uapi/linux/netfilter/xt_rateest.h   | 15 ++++++++---
> include/uapi/linux/netfilter/xt_tcpmss.h    | 12 ++++++---
> include/uapi/linux/netfilter_ipv4/ipt_ECN.h | 29 ++-------------------
> include/uapi/linux/netfilter_ipv4/ipt_TTL.h | 25 ++++--------------
> include/uapi/linux/netfilter_ipv4/ipt_ecn.h | 26 ++++++++++++++++++
> include/uapi/linux/netfilter_ipv4/ipt_ttl.h | 23 +++++++++++++---
> include/uapi/linux/netfilter_ipv6/ip6t_HL.h | 26 ++++--------------
> include/uapi/linux/netfilter_ipv6/ip6t_hl.h | 22 +++++++++++++---
> net/ipv4/netfilter/ipt_ECN.c                |  2 +-
> net/netfilter/xt_DSCP.c                     |  2 +-
> net/netfilter/xt_HL.c                       |  4 +--
> net/netfilter/xt_RATEEST.c                  |  2 +-
> net/netfilter/xt_TCPMSS.c                   |  2 +-
> 21 files changed, 143 insertions(+), 144 deletions(-)

Technically you split up your single patch into multiple parts but not 
separated it into functionally disjunct parts. So please prepare

- one patch for
 	include/uapi/linux/netfilter_ipv6/ip6t_HL.h
 	include/uapi/linux/netfilter_ipv6/ip6t_hl.h
 	net/netfilter/xt_HL.c
 	net/netfilter/xt_hl.c
 	[ I'd prefer corresponding Kconfig and Makefile changes as well]
- one patch for
 	include/uapi/linux/netfilter/xt_RATEEST.h
 	include/uapi/linux/netfilter/xt_rateest.h
 	net/netfilter/xt_RATEEST.c
 	net/netfilter/xt_rateest.c
 	[I'd prefer corresponding Kconfig and Makefile changes as well]
- and so on...

That way the reviewers can follow what was moved from where to where in a 
functionally compact way.

Also, mechanically moving the comments results in text like this:

> /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> -/* ip6tables module for matching the Hop Limit value
> +/* Hop Limit modification module for ip6tables
> + * ip6tables module for matching the Hop Limit value

which is ... not too nice. The comments need manual fixing.

I also still don't like adding pragmas to emit warnings about deprecated 
header files. It doesn't make breaking API easier and it doesn't make 
possible to remove the warnings and enforce the changes just after a few 
kernel releases.

Best regards,
Jozsef