Re: [PATCH v4] netlink: Fix nl_route_dup by 3-step processing

[Martin Schitter <ms+git@mur.at>]

On 11/7/26 21:02, Stefano Brivio wrote:
> On Sat, 7 Nov 2026 12:50:18 +0100
> Martin Schitter <ms+git@mur.at> wrote:
> 
>> On 10/21/26 01:56, Stefano Brivio wrote:
>>>> Fixes: #221
[...]
> There's also the little problem that (same here, this comes from the
> Linux kernel process) we use "Fixes" tags to indicate previous commits
> that are fixed with a given change, rather than links to bugs, so that
> would kind of overlap, and it wouldn't be possible for me (and possibly
> others) to use git hooks such as the one I shared here:

That's exactly the way how it works in all common CI/CD solutions as 
well. They also use this key-value-pair whenever a patch/PR/MR becomes 
accepted and merged into the main branch and will utilize the 
information for various automatized tasks (closing the issue etc.).

>>>> +/**
>>>> + * nl_rtattr_cmp() -- compare two routing attributes
>>>
>>> But what about returning a bool (we use C11 now) and turn this into
>>> nl_rtrattr_eq(), so that one doesn't have to wonder if it behaves like
>>> memcmp() or the other way around? There's a single non-zero value we
>>> return anyway.
>>
>> yes -- it definitely could be done this way as well.
>> But isn't it more useful to keep this code sequence as close as possible
>> to its origin in iproute2?
> 
> Hmm, I see your point.  I'd still tend to say not because it's more
> important to make this easier to grasp inside the codebase itself, and
> we almost never have to refer to iproute2 implementations, but that was
> a very slight preference on my side.

Yesterday, when I reformulated these logical filters to pick route 
entries corresponding to one of the three transfer stages, I recognized 
that some checks (e.g. testing for existence of SRC entries) are in fact 
redundant given this actual implementation of this compare routine, and 
the other way around some parts of the tests here will never be actually 
reached in the actual execution flow.

Nevertheless, it still makes sense to write it down in this slightly 
redundant fashion to make the decision semantic easier graspable and 
prevent errors if someone will modify parts of the code in the future.

Duplicating the route tables without dynamic memory allocation and all 
its dependency challenges is such a tricky problem, that someone will 
very likely have to improve it again sooner or later and then perhaps 
also profit again from future modifications, bug reports and fixes in 
the corresponding sections of iproute2.

>>>> +			.rta.rta_type = RTA_OIF,
>>>> +			.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)),
>>>> +			.ifi = ifi_src,
>>>
>>> The way this was indented before (with tabs) looked more readable,
>>> because it built some kind of "table".
>>
>> It was using an unpleasant mix of tabs and space characters, which will
>> not always look nice and better readable in all possible code views.
> 
> If tabs are 8 characters wide (just like the kernel coding style that
> we use), then it all works and it's consistent.

It's a rather controversial topic.

For example, in a diff, when lines get prefixed with marks that shift 
everything to the right, the alignment will very likely break again. The 
same will happen in code displays on the web, that usually ignore all 
this project specific style guidelines and expected tab width, etc. ...

My editor (zed) also doesn't display the actual column in the status 
bar, but instead the number of characters used up to the current cursor 
position, which unfortunately isn't equal to the column number whenever 
tabs are used. This makes it harder to control the rendered line width, 
but it still makes a lot of sense whenever you want to operate on text 
positions by descriptive means.

I personally don't care so much about the different dogmatic positions 
concerning style and superfluous code cosmetics, but I really like the 
way how this issue is handled by `cargo-fmt` in rust land. A fully 
automatic cleanup solution for most of this style related decisions that 
got accepted by nearly all developers resp. the vast majority of 
projects, which simply eliminates lengthy discussions about this topic.

>> Well -- as far as I can see, all modifications of v5 are only of
>> cosmetic nature and should not alter the actual operation.
> 
>  From a very quick look, I think so too.  I need to have another calmer
> look but I think it's ready to be merged as it is.

That's fine!

I really appreciate your careful reviews, because even in case of rather 
boring cosmetic modifications I could have made a mistake.
>> I'm really glad you're putting so much effort into reviewing this
>> patches, but please just go ahead and change this less important
>> stylistic aspects on the fly without unnecessary debates.
> 
> Okay, I will, if "needed".

thanks!

>> Currently, we
>> all have to fight the burden of stupid LLM generated slop, but we also
>> still have to feel responsible for handling human collaborators not like
>> willless agents and bots. Insisting on overly picky style guides can be
>> really frustrating for voluntary contribution.
> 
> Oops, my bad, I'm sorry you felt that way.  You already mentioned on an
> earlier version that I could just go ahead and fix up coding style, but
> I had more substantial comments than that on v4, so I thought I'd just
> give you all my comments anyway.  In some spots, I didn't want to tweak
> your code without even asking for your opinion.

Please don't take it as an insult!

In fact, I really like the way how you handle this job, communicate and 
check everything noticeable carefully.

> And given that this fix is a bit more than a fix, and genuinely valuable
> development, spending a bit of time on coding style comments and
> related explanations makes sense an investment for me, because I hope
> you'll contribute further in the future, and in that case that would
> save me time.

For me, it's just a matter of being a responsible net citizen to write 
useful bug reports and occasionally contribute fixes. It's more or less 
the price we should all have to pay for using all this wonderful 
treasures of free software.

> In general, I'm trying to be picky about style because it helps _all_
> contributors in the long term.
Yes, it's definitely worth to keep the code base clean and consistent, 
but it's much easier to keep it that way if you are more familiar with 
the project and work in an already prepared and correct configured 
development setup.

thanks
martin