Message-ID: <33196cbc-2763-48d5-9e26-7295cd70b2c4@freemail.hu>
Date: Mon, 6 Jan 2025 13:41:55 +0100
From: Szőke Benjamin <egyszeregy@...email.hu>
To: Jozsef Kadlecsik <kadlec@...ckhole.kfki.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.

2025. 01. 06. 9:19 keltezéssel, Jozsef Kadlecsik írta:
> 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.

First suggestion was to split it 2 parts, it is done, i split in 3 parts, it was 
more then needed. Your idea will lead to split it about to 20 patch parts, then 
the next problem from you could be "there are to many small singel patches, 
please reduce it".

If you like to see it in a human readable format you can found the full diff and 
the separted patches also in this link:
https://github.com/torvalds/linux/compare/master...Livius90:linux:uapi

Please start to use any modern reviewing tool in 2025 and you can solve your 
problem. In GitHub history view i can see easly what was moved from where to 
where in 1-3 mouse clicking, eg.: click to xt_DSCP.h then click to xt_dscp.h and 
you can see everything nicely. So it is ready for reviewing, please sit down and 
start work on it as a maintainer, It's your turn now.

https://github.com/torvalds/linux/commit/1ee2f4757ff025b74569cce922147a6a8734b670

> 
> 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 do not know what small and compact "title" should be good here in the merged 
header files. Most simplest solution was to copy paste them and merge these 
titles text.

You should know it better, please send a new compact and perfectly good "title" 
text for all header files which are in the patchset and i can change them 
finally. I think it is out of my scope in this business.

> 
> 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.

I also still like adding pragmas, because duplicating these header files is not 
acceptable in SW dev/coding. It must have to be taught for the user how should 
use it in the future. This is a common way in any SW, for example Python or 
Matlab always send a notice in run-time for you which will be a deprecated 
things soon, when you import or start to use an old function or module.

Why don't you think it can not help breaking API easier? This is the bare 
minimum what you can do for it. Tell to user what should use instead, then 3-5 
years later you can change it finally, when 90-95% percent of your customers 
learnt to it and already started to use it in their userspace codes.

> 
> Best regards,
> Jozsef

Powered by blists - more mailing lists