multipath-tcp / mptcpd

mptcpd is a daemon for Linux that performs Multipath TCP path management related operations in the user space 😈
https://mptcpd.mptcp.dev/
BSD 3-Clause "New" or "Revised" License
181 stars 38 forks source link

Support ELL >=0.69 #302

Closed jonassmedegaard closed 2 weeks ago

jonassmedegaard commented 2 months ago

ELL has dropped two symbols since version 0.69: l_genl_attr_next and _genl_attr_recurse.

At first I just sloppily assumed they knew what they were doing and that change was harmless, since they didn't bump the API, but releasing the new version of ELL to Debian immediately made iwd fail, and a search revealed that also mptcpd directly rely on those symbols.

For inspiration, here's the change iwd made to get rid of those symbols: https://git.kernel.org/pub/scm/network/wireless/iwd.git/commit/?id=77cf621

matttbe commented 2 months ago

Hi @jonassmedegaard,

Thank you for the bug report.

since they didn't bump the API

Did they do that with version 0.68? Because mptcpd also needs to be adapted to support it, see #299.

Maybe they don't know/remember they have to bump the API :) Also, I think people behind libell and iwd are the same ones, no? Maybe they forgot ell was used by other projects :)

I guess the changes should not be too important, but personally, I don't know that code and I have deadlines approaching :-/ I'm not sure if I can get some support from @ossama-othman, but I can see what is possible to do here. For Debian, when should it ideally be fixed? I guess it will soon be blocking new iwd versions, no?

ossama-othman commented 2 months ago

@matttbe I’ll take a look at the updates necessary to support the ELL API tonight.

jonassmedegaard commented 2 months ago

Thanks for for quick reactions!

It is not currently blocking iwd, because I messed up and went ahead upgrading both libell and iwd - so it is kinda worse: If I don't get a fix for mptcpd then I'll need to roll back those releases with ugly dummy superseding versions clamped on ( 0.69.really0.68) and then be blocked...

matttbe commented 2 months ago

@ossama-othman thank you :)

@jonassmedegaard I see. Hopefully the changes will not be too important. I had a quick look on ELL mailing list about the API changes, and I didn't see anything. If you already planned to contact them, do not hesitate to tell them to avoid breaking the API if possible, or at least to bump the version accordingly (and document the changes :) ).

ossama-othman commented 2 months ago

The change to ELL in the 0.69 release does indeed break link-time compatibility but the removed symbols still exist as static inline functions, meaning recompiling code that uses those functions, such as iwd and mptcpd, should address the issue.

AFAICT, the iwd change you referred to doesn't directly avoid those symbols since they are still used elsewhere in the iwd nl80211util.c source file.

matttbe commented 2 months ago

@jonassmedegaard: thanks to @ossama-othman's work, mptcpd can now be compiled with ELL's development version again. Does his patch fix the issue you saw on Debian side?

matttbe commented 2 weeks ago

The change to ELL in the 0.69 release does indeed break link-time compatibility but the removed symbols still exist as static inline functions, meaning recompiling code that uses those functions, such as iwd and mptcpd, should address the issue.

@ossama-othman I tried to recompile mptcpd 0.12 with your patch from #303, and I got the following errors:

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I../include/mptcpd/private -I../include -I../include -DMPTCPD_DLL -DMPTCPD_DLL_EXPORTS -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -Werror=implicit-function-declaration -ffile-prefix-map=/build/mptcpd-0.12=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection -Wall -pedantic -fvisibility=hidden -Wextra -Werror -Wformat=2 -c path_manager.c  -fPIC -DPIC -o .libs/libmptcpd_la-path_manager.o
In file included from path_manager.c:21:
/usr/include/ell/genl.h: In function 'l_genl_attr_next':
/usr/include/ell/genl.h:98:16: error: implicit declaration of function 'l_netlink_attr_next'; did you mean 'l_genl_attr_next'? [-Wimplicit-function-declaration]
   98 |         return l_netlink_attr_next((struct l_netlink_attr *) attr,
      |                ^~~~~~~~~~~~~~~~~~~
      |                l_genl_attr_next
/usr/include/ell/genl.h: In function 'l_genl_attr_recurse':
/usr/include/ell/genl.h:105:16: error: implicit declaration of function 'l_netlink_attr_recurse'; did you mean 'l_genl_attr_recurse'? [-Wimplicit-function-declaration]
  105 |         return l_netlink_attr_recurse((struct l_netlink_attr *) attr,
      |                ^~~~~~~~~~~~~~~~~~~~~~
      |                l_genl_attr_recurse
make[2]: *** [Makefile:597: libmptcpd_la-path_manager.lo] Error 1

It looks like an issue on ELL header files, no?

matttbe commented 2 weeks ago

Apparently, mptcpd should only include ell/ell.h files, and not individual ones. See #309