ospf6d: factor out lsdesc iterator #15725
Conversation
acooks-at-bda
force-pushed
the
factor-out-lsdesc-iterator
branch
from
4f4e2ad
to
3ec5c18
Compare
| @@ -452,25 +436,15 @@ static bool ospf6_gr_check_adjs_lsa_transit(struct ospf6_area *area, | |||
| return true; | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Remove added whitespace.
ospf6d/ospf6_gr.c
Outdated
| #define lsa_from_container(type, container) \ | ||
| ((struct type *)((char *)container->header + sizeof(struct ospf6_lsa_header))) | ||
|
|
||
| #define lsdesc_start(lsdesc_type, lsa) \ |
There was a problem hiding this comment.
How about prefixing these macro names with "lsa_link_"? For example, "lsa_link_lsdesc_start(). While the naming in ospf6d is not ideal, no sense in making it even more cryptic.
|
Is there some greater reason to change the code this way? Ie, is there some greater change coming that needs these iterators to be macrotized? Or is this just being done to compact the code and reduce repetition? |
I'm not suggesting this is the merge-able solution, but imo there is a worthwhile improvement to be made to the code that iterates over TLVs and LSAs. I'm working towards an implementation of RFC 8362 - OSPFv3 Link State Advertisement (LSA) Extensibility, which defines new LSAs in terms of TLV types, and will require iterators over TLVs in more places. That will be a significant functional change, which I may have to maintain as a patch for some time, so I'm trying to get early feedback on small patches. Thanks. |
This is certainly needed since all the later OSPFv3 features (include SR and SRv6) are dependent on OSPFv3 Extended LSA support. |
|
I believe the code clean up proposal that it suggested is a good idea. For sure, it'll be a major change for ospf6d, but it'll help to have a more mature code. @acooks-at-bda : could you explain your strategy for those changes ? For sure, it'll prevent doing some backports and maintenances but on the other side, we need to move ospf6d to become more mature. |
Thank you, that is what I was after, so this does fit into a grander plan, and with that information others have agreed that this is a worthwhile thing to do despite the issues it may raise with backporting. I am going to suggest to mark this pull request as WIP for now, and then proceed as @aceelindem aceelindem suggested:
|
acooks-at-bda
force-pushed
the
factor-out-lsdesc-iterator
branch
from
3ec5c18
to
b2210a1
Compare
|
I've rebased this on the work in PR #16055, which unfortunately makes it hard to see what's new. Basically the iterators depend on the ospf6_lsa_end() function introduced in #16055 and if the order of the two PRs were reversed, there would still be a dependency between them. I am trying to control the amount of churn, while showing where I'm heading with this. Some churn just seems unavoidable. |
acooks-at-bda
force-pushed
the
factor-out-lsdesc-iterator
branch
from
b2210a1
to
d881e60
Compare
Replacing the macro with an inline function allows the compiler to check the parameter type. Use the replacement function consistently to reduce the number of open coded pointer cast plus offset calculations. use tools/indent.py to reformat all occurences of its use. Signed-off-by: Andrew Cooks <[email protected]>
Replacing the macro with an inline function enables better type checking. No functional change. Signed-off-by: Andrew Cooks <[email protected]>
Dropping the macro enables better compiler type checking. The macro was not used consistently when reading the lsa size from the header, so this change also aims to use the replacement inline function consistently. Keeping the inline function has (marginal) utility in that it ensures that the endian conversion is consistently performed. Signed-off-by: Andrew Cooks <[email protected]>
Adds iterator macros for the descriptors in an LSA, to replace the number of repeated open coded pointer casts and pointer arithmetic. Signed-off-by: Andrew Cooks <[email protected]>
Replace open coded iterators with new lsdesc iterator macro Signed-off-by: Andrew Cooks <[email protected]>
acooks-at-bda
force-pushed
the
factor-out-lsdesc-iterator
branch
from
d881e60
to
7873b67
Compare
Add a macro to iterate over the descriptors in an LSA and refactor the existing open coded iterators.
Other parts of ospf6d also iterate over
struct ospf6_*_lsdescand similar types, and perhaps a generic solution for all the instances can be found.