Message-ID: <f1f0e509-d14f-3731-0b44-3727dfdf18d9@blackhole.kfki.hu>
Date: Mon, 6 Jan 2025 14:40:07 +0100 (CET)
From: Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>
To: Szőke Benjamin <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, Szőke Benjamin wrote:

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

It'd mean 8 patches according to the merged match/TARGET files: mark/MARK, 
connmark/CONNMARK, dscp/DSCP, rateest/RATEEST, tcpmss/TCPMSS, ecn/ECN, 
ttl/TTL, hl/HL. Each one of them would be a unit which then could be 
reviewed, tested independently all of the other ones.

> 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

Thanks the suggestion: still, all changes are lumped together and cannot 
be handled separatedly.

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

It's pretty trivial in the example: "ip6tables module for 
matching/modifying the Hop Limit value". But any automated merging needs 
manual verifying and fixing if needed.

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

Sorry, but no: it's your responsibility to produce proper patches,
including the modified comments.

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

However as far as I'm concerned, breaking API is not a decided and 
accepted thing. Breaking API in the kernel is not a "normal business" at 
all.

Best regards,
Jozsef

Powered by blists - more mailing lists