Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

route: add missing rtnl_nh_get_oif symbol #419

Closed

Conversation

KanjiMonster
Copy link
Contributor

@KanjiMonster KanjiMonster commented Feb 4, 2025

When nh support was added in 780d06a ("route: add nh type"), rtnl_nh_get_oif() was missed adding to libnl-route-3.sym.

Add the missing symbol so it can be actually used.

Fixes: 780d06a ("route: add nh type")

@KanjiMonster
Copy link
Contributor Author

I added them alongside the other rtnl_nh_* as that is when they were added, though I'm not sure if this is the right place to add them, or if they should be in libnl_3_12.

 /usr/bin/ld: rtnl_nh_dump: undefined version: libnl_3_8
/usr/bin/ld: rtnl_nh_clone: undefined version: libnl_3_8

That took me a while to reproduce. So these functions actually don't exist.

@KanjiMonster KanjiMonster marked this pull request as draft February 4, 2025 14:04
@KanjiMonster KanjiMonster force-pushed the jogo_missing_rtnl_nh_symbols branch from a845d4e to 570be1d Compare February 4, 2025 14:07
@KanjiMonster KanjiMonster changed the title route: add missing rtnl_nh_* symbols route: add missing rtnl_nh_get_oif symbol Feb 4, 2025
@KanjiMonster
Copy link
Contributor Author

Ah, a commit post 3.11 dropped the definitions, that explains it (started working on this based on 3.11). Dropped and updated.

@KanjiMonster KanjiMonster marked this pull request as ready for review February 4, 2025 14:09
@thom311
Copy link
Owner

thom311 commented Feb 4, 2025

The linker symbols help with versioning. For example, RPM package dependencies automatically tracks when a certain package requires libnl@libnl3_12 symbol. This means, you can never add a symbol to e.g. libnl3_7 after 3.7 is released.

The correct solution is to add the symbols now to libnl3_12 (the bottom of the file). There is no going back in time to fix 3.7. This is fine, and the right solution.

3.12 because that is the next upcoming release.

Add a new libnl3_12 section, if it doesn't exist yet.

@KanjiMonster
Copy link
Contributor Author

Well, one could do a 3_8_1 that fixes it ;P

But unless that happens, I guess 3_12 is the next best thing.

@KanjiMonster KanjiMonster force-pushed the jogo_missing_rtnl_nh_symbols branch from 570be1d to 520b1d5 Compare February 4, 2025 14:14
When nh support was added in 780d06a ("route: add nh type"),
rtnl_nh_get_oif() was missed adding to libnl-route-3.sym.

Add the missing symbol so it can be actually used.

Fixes: 780d06a ("route: add nh type")
Signed-off-by: Jonas Gorski <[email protected]>
@KanjiMonster KanjiMonster force-pushed the jogo_missing_rtnl_nh_symbols branch from 520b1d5 to f752122 Compare February 4, 2025 14:15
@thom311
Copy link
Owner

thom311 commented Feb 4, 2025

Well, one could do a 3_8_1 that fixes it ;P

Yes, but I don't maintain a 3.8 stable branch, so we would never do a release that only brings libnl3_8_1 without libnl3_12.

If we would maintain old-stable branches then yes. Because then the set of libnl3_8_1 could be backported individually (when backporting symbols, we must backport all symbols of a certain libnl3_X version).

@KanjiMonster
Copy link
Contributor Author

KanjiMonster commented Feb 4, 2025

Righto. Anyway, moved it to libnl3_12, and made it based on libnl3_11, not libnl3_10 *coughs*.

thom311 pushed a commit that referenced this pull request Feb 4, 2025
When nh support was added in 780d06a ("route: add nh type"),
rtnl_nh_get_oif() was missed adding to libnl-route-3.sym.

Add the missing symbol so it can be actually used.

Fixes: 780d06a ("route: add nh type")
Signed-off-by: Jonas Gorski <[email protected]>

#419
@thom311
Copy link
Owner

thom311 commented Feb 4, 2025

merged as 470c0a6

thank you!!!

@thom311 thom311 closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants