Re: [pve-devel] [PATCH frr] frr: fix bit flag collision in patch

[Gabriel Goller <g.goller@proxmox.com>]

On 13.03.2025 16:16, Thomas Lamprecht wrote:
>On 13/03/2025 13:49, Gabriel Goller wrote:
>> Resolve conflict between F_ISIS_UNIT_TEST and ISIS_OPT_DUMMY_AS_LOOPBACK
>> which were both using the same bit value (0x01). This collision caused
>> unit test mode to be unintentionally enabled when DUMMY_AS_LOOPBACK was set.
>>
>
>This is also wrong at upstream then though?
>
>https://github.com/FRRouting/frr/blob/master/isisd/isisd.h#L79-L81
>
>Is upstream notified of this? maybe send a PR to them if testing holds
>up.

Already done, forgot to mention this:
https://github.com/FRRouting/frr/pull/18377

>missing a fixes trailer, like:
>
>Fixes: ecf591e ("frr: add the dummy_as_loopback patch series, enable it by default")

Right!

>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>
>> * I'm not sure about the debian version number, let me know if this is
>>   wrong.
>
>For starters please always do the bump in a separate patch, it's rather
>unrelated to the fix. I'm also fine with handling bumps myself, as should be
>all release team members. FWIW, I appreciate getting changes added there,
>and thought about even starting to requesting that, but they really need
>to target humans and should keep the "UNRELEASED" as value for the release,
>i.e. what the `dch -i` template uses by default.

Ok, will do.

>w.r.t. versioning I'd have bumped the pve1 part to pve2.

So '10.2.1-1+pve2'?
Makes sense as we deviate from the debian package here.

>> * I edited the patch introducing this bug, if it's better to add a new
>>   patch, I'll be happy to change it.
>
>This is fine, layered patches of patches are seldomly a good option.
>
>> * We are still testing the newest version so it's not yet ready for
>>   testing/no-subscription.
>
>So what can I expect here, should it still be packaged and uploaded to
>internal staging for more broad QA?

Yep, internal staging is good. Just didn't want you to think of this
patch as a "we're done with testing" message :)

>>
>>  debian/changelog                                    |  6 ++++++
>>  ...d-option-to-treat-dummy-interfaces-as-loop.patch | 13 ++++++++-----
>>  2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/debian/changelog b/debian/changelog
>> index 6e68311132b1..7e2ffd7964a9 100644
>> --- a/debian/changelog
>> +++ b/debian/changelog
>> @@ -1,3 +1,9 @@
>> +frr (10.2.1-2+pve1) bookworm; urgency=medium
>> +
>> +  * fix fabricd dummy_as_loopback flag collision
>
>collision with what? these entries should be telling for end users (not devs).

True, a simpler "fix fabricd dummy_as_loopback flag" would be enough.

>> +
>> + -- Gabriel Goller <g.goller@proxmox.com>  Thu, 13 Mar 2025 13:33:46 +0100
>> +
>>  frr (10.2.1-1+pve1) bookworm; urgency=medium
>>
>>    * update upstream source to 10.2.1
>> diff --git a/debian/patches/pve/0005-fabricd-add-option-to-treat-dummy-interfaces-as-loop.patch b/debian/patches/pve/0005-fabricd-add-option-to-treat-dummy-interfaces-as-loop.patch
>> index 331beed378ec..184a093334c9 100644
>> --- a/debian/patches/pve/0005-fabricd-add-option-to-treat-dummy-interfaces-as-loop.patch
>> +++ b/debian/patches/pve/0005-fabricd-add-option-to-treat-dummy-interfaces-as-loop.patch
>> @@ -18,8 +18,8 @@ Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>>   isisd/isis_circuit.c         |  9 +++++----
>>   isisd/isis_main.c            | 16 +++++++++++++---
>>   isisd/isisd.c                | 19 +++++++++++++++++++
>> - isisd/isisd.h                |  4 ++++
>> - 6 files changed, 57 insertions(+), 12 deletions(-)
>> + isisd/isisd.h                |  6 +++++-
>> + 6 files changed, 58 insertions(+), 13 deletions(-)
>>
>>  Index: b/doc/manpages/frr-fabricd.rst
>>  ===================================================================
>> @@ -181,15 +181,18 @@ Index: b/isisd/isisd.h
>>  ===================================================================
>>  --- a/isisd/isisd.h	2025-03-07 11:09:47.700424235 +0100
>>  +++ b/isisd/isisd.h	2025-03-07 11:09:47.698424233 +0100
>> -@@ -74,7 +74,9 @@
>> +@@ -74,9 +74,11 @@
>>   	struct list *isis;
>>   	/* ISIS thread master. */
>>   	struct event_loop *master;
>>  +	/* Various global options */
>>   	uint8_t options;
>> -+#define ISIS_OPT_DUMMY_AS_LOOPBACK (1 << 0)
>> ++#define F_ISIS_UNIT_TEST (1 << 0)
>> ++#define ISIS_OPT_DUMMY_AS_LOOPBACK (1 << 1)
>
>somewhat pre-existing, but yuck, who places defines in the middle of struct
>definitions?!¿

Stefan said the exact same thing :)
This is done quite commonly in frr e.g.:
https://git.proxmox.com/?p=mirror_frr.git;a=blob;f=bgpd/bgpd.h;h=9cb1d51088cfc456f344b17b8068f84d382e3751;hb=HEAD#l210.
But I don't think it's that bad anyway :).

>btw. such bit flag definition are one of the few cases where aligning the
>values makes sense and the header you're modifying here already uses that
>style:
>
>https://github.com/FRRouting/frr/blob/master/isisd/isisd.h#L374-L389
>

Agree.

>>   };
>> - #define F_ISIS_UNIT_TEST 0x01
>> +-#define F_ISIS_UNIT_TEST 0x01
>> +
>> + #define ISIS_DEFAULT_MAX_AREA_ADDRESSES 3
>>
>>  @@ -269,6 +271,8 @@
>>   void isis_terminate(void);

Thanks for looking at this!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel