ell: only include 'ell/ell.h' header

[matttbe]

When looking at the code of other projects using ELL (IWD, BlueZ, Ofono), it looks like only 'ell.h' should be included, not individual header files from the 'ell' header directory.

That looks like the way to go, because when looking at ell/genl.h, it uses functions declared in ell/netlink.h, without including this file before. This causes issues when compiling the code using libell-dev installed on the system:

libtool: compile:  gcc (...) -c path_manager.c (...)
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

All .c files including ELL header files have been modified to include only <ell/ell.h>. The .cpp file in the tests has not been modified, because it looks like that causes some issues. For the same reason, include/mptcpd/private/plugin.h file has not been modified as well.

While at it, we can also remove the pragma to ignore -Wpedantic as it looks like it was caused by the fact ell/ell.h was not used.

Closes: #302

[coveralls]

Pull Request Test Coverage Report for Build 11531189571

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 64.799%

---

Totals
Change from base Build 11503089273:	0.0%
Covered Lines:	1399
Relevant Lines:	2159

---

💛 - Coveralls

[ossama-othman]

I'm not fond of convenience headers like ell/ell.h that expose all library APIs through a single header. I instead prefer to only include what you need. For example, if I only need what is in ell/util.h why should I indirectly include the other unnecessary 48 headers pulled in by ell/ell.h? That is why mptcpd has avoided including ell/ell.h from the start. I understand including ell/ell.h is the supported way to use the ELL API, and that there are other libraries that do the same thing (e.g. glib/glib.h), but I still don't think that's the best way to go. :)

For cases like ell/genl.h needing symbols in ell/netlink.h the header containing the missing symbols should generally be included, assuming forward declarations would not solve the problem, to allow it to be self-contained, rather than having to rely on headers that include all public API headers. The right thing to do, IMO, in this case is to send a patch to the ELL maintainers that makes ell/genl.h include ell/netlink.h.

If you really want to include ell/ell.h in mptcpd to avoid such issues I won't push back any more than this, and will approve the PR. Let me know. 😉

[matttbe]

Hi @ossama-othman,

Thank you for your reviews!

I'm not fond of convenience headers like ell/ell.h that expose all library APIs through a single header. I instead prefer to only include what you need. For example, if I only need what is in ell/util.h why should I indirectly include the other unnecessary 48 headers pulled in by ell/ell.h? That is why mptcpd has avoided including ell/ell.h from the start. I understand including ell/ell.h is the supported way to use the ELL API, and that there are other libraries that do the same thing (e.g. glib/glib.h), but I still don't think that's the best way to go. :)

I agree with you. I don't like this way, and I think it is better to have self-contained header files.

For cases like ell/genl.h needing symbols in ell/netlink.h the header containing the missing symbols should generally be included, assuming forward declarations would not solve the problem, to allow it to be self-contained, rather than having to rely on headers that include all public API headers. The right thing to do, IMO, in this case is to send a patch to the ELL maintainers that makes ell/genl.h include ell/netlink.h.

I initialled looked at that -- in fact, my first intention was to only modify mptcpd first, to include ell/netlink.h before ell/genl.h, but that was not enough, and I ran into other issues -- but when I looked at ell's header files, I saw that they were really not self-contained, and more than just that would be needed.

If you really want to include ell/ell.h in mptcpd to avoid such issues I won't push back any more than this, and will approve the PR. Let me know. 😉

Honestly, I would prefer not to use ell/ell.h, but I think that would be better to do that, to avoid similar issues in the future. So I'm suggesting this change mainly to ease the future maintenance.