From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=fail (p=none dis=none) header.from=mur.at Received: from efeu.mur.at (efeu.mur.at [89.106.208.42]) by passt.top (Postfix) with ESMTPS id 752EF5A026E for ; Sun, 08 Nov 2026 12:18:08 +0100 (CET) Received: from [192.168.4.117] (lan1.raspi.ma39.ffgraz.net [10.12.1.243]) by efeu.mur.at (Postfix) with ESMTPSA id 7491D42472; Sun, 8 Nov 2026 12:18:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mur.at; s=dkim2; t=1794136687; bh=6RBFnVwxLxY68b3gX2ANaZH+QdEnempGhyj0l7I08Cw=; h=Date:From:Subject:To:Cc:References:In-Reply-To:From; b=FaQt7nrUS+nVTOTkxzvDM/whCA+DlO7HRybktVejIVukBAVTxGUBgHg/qtBp5Kfg2 Ayf84hu/xlhgEyNdWumQsS22GkfTCP067pilekLZxrAUALfqDjcIiovb4ulR51jUpW zeDUKUq0AM4/WPqINBSZdTOvNNn1gUGWP+sR3Lm6z5PVpSCyd+e7BwV6xfD0nrchwl S8TBiNvyxN1DUjgm1yLDyG7xR0oQG8lfMY+6k3ETJpY6nLDxKblgwTmZm1yx3YMgTw m3/32GGp/7IjlgczXAQohNYTztmUuMIa51wSsEYSMN4ISZrnB1mzllBXJTqE2QAWNq MhQm6fSXnlavZOeOgW56OMo2LOVHQYtBMZ6WpceYHtfGRXaCUYfWKtN+QjSgU7PERg HS919oRhSwz/K6Ck9YQ3Wt7uAhRiSywyjxv4Bl/uoy4YfebTkXuRs9UzIZzpaOcQx/ BF3O7wVPXBqFrW+ckPhdULmE43wUcPYrFMHtxIzm6qgpeNCXd1w Message-ID: Date: Sun, 8 Nov 2026 12:18:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Martin Schitter Subject: Re: [PATCH v4] netlink: Fix nl_route_dup by 3-step processing To: Stefano Brivio References: <20260926120520.809653-1-ms+git@mur.at> <20260926120520.809653-2-ms+git@mur.at> <20261021015651.582339bd@elisabeth> <20261107210237.59bcb1cf@elisabeth> Content-Language: en-US In-Reply-To: <20261107210237.59bcb1cf@elisabeth> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-MailFrom: ms+git@mur.at X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation Message-ID-Hash: RYNWY45DOISMZOMCFXGYEAPBWCTR7BP4 X-Message-ID-Hash: RYNWY45DOISMZOMCFXGYEAPBWCTR7BP4 X-Mailman-Approved-At: Sun, 08 Nov 2026 12:29:24 +0100 CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 11/7/26 21:02, Stefano Brivio wrote: > On Sat, 7 Nov 2026 12:50:18 +0100 > Martin Schitter 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