Skip to content

Use pkg-config to discover libnl-3 #1637

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.

Sign up for GitHub

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

Open
fpletz wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use pkg-config to discover libnl-3 #1637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
966d515
Use pkg-config to find libnl-3 headers
fpletz Mar 12, 2025
40dcb84
Dynamically load libnl-3 libs from pkg-config libdir
fpletz Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,14 +961,16 @@ case "$enable_delayacct" in
enable_delayacct=no
else
old_CFLAGS="$CFLAGS"
CFLAGS="$CFLAGS -I/usr/include/libnl3"
htop_config_cflags=`$PKG_CONFIG --cflags libnl-3.0 2>/dev/null` || continue
Copy link
Contributor

@Explorer09 Explorer09 Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish some quality improvements to the check:

  1. There's no error handling here. What would you do if the pkg-config call fails?
  2. Is there any reason not to use PKG_CHECK_MODULES macro as in the old code?
  3. You should allow the user to override the compiler flags detected. If you have used PKG_CHECK_MODULES, then the variable LIBNL3_CFLAGS would be defined automatically to allow that user override. Otherwise, maybe you can define it yourself? (AC_ARG_VAR([LIBNL3_CFLAGS], [C compiler flags for libnl3, overriding pkg-config]))

Copy link
Contributor Author

@fpletz fpletz Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no error handling here. What would you do if the pkg-config call fails?

In this case the AC_CHECK_HEADERS fails like it would have done before since htop_config_cflags is empty and the headers aren't found. But PKG_CHECK_MODULES should take care of that.

Is there any reason not to use PKG_CHECK_MODULES macro as in the old code?

I used the current style since that is used to discover ncurses in the same file. I agree that PKG_CHECK_MODULES is the better solution and will change the code accordingly.

You should allow the user to override the compiler flags detected. If you have used PKG_CHECK_MODULES, then the variable LIBNL3_CFLAGS would be defined automatically to allow that user override. Otherwise, maybe you can define it yourself? (AC_ARG_VAR([LIBNL3_CFLAGS], [C compiler flags for libnl3, overriding pkg-config]))

Yes, makes sense. I will add this. And also introduce the variable LIBNL3_LIBDIR.

Copy link
Contributor

@Explorer09 Explorer09 Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason the ncurses detection code doesn't use PKG_CHECK_MODULES is that ncurses may come with many different library names (as well as .pc file names), so the code has to implement fallbacks.

Copy link
Member

@BenBE BenBE Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS, this should be || true, as there is no loop.

Suggested change
htop_config_cflags=`$PKG_CONFIG --cflags libnl-3.0 2>/dev/null` || continue
htop_config_cflags=`$PKG_CONFIG --cflags libnl-3.0 2>/dev/null` || true

CFLAGS="$CFLAGS $htop_config_cflags"
AC_CHECK_HEADERS([netlink/attr.h netlink/handlers.h netlink/msg.h], [enable_delayacct=yes], [enable_delayacct=no])
CFLAGS="$old_CFLAGS"
fi
;;
yes)
old_CFLAGS="$CFLAGS"
CFLAGS="$CFLAGS -I/usr/include/libnl3"
htop_config_cflags=`$PKG_CONFIG --cflags libnl-3.0 2>/dev/null` || continue
Copy link
Member

@BenBE BenBE Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here …

Suggested change
htop_config_cflags=`$PKG_CONFIG --cflags libnl-3.0 2>/dev/null` || continue
htop_config_cflags=`$PKG_CONFIG --cflags libnl-3.0 2>/dev/null` || true

CFLAGS="$CFLAGS $htop_config_cflags"
AC_CHECK_HEADERS([netlink/attr.h netlink/handlers.h netlink/msg.h], [], [AC_MSG_ERROR([can not find required header files netlink/attr.h, netlink/handlers.h, netlink/msg.h])])
CFLAGS="$old_CFLAGS"
;;
Expand All @@ -978,7 +980,10 @@ case "$enable_delayacct" in
esac
if test "$enable_delayacct" = yes; then
AC_DEFINE([HAVE_DELAYACCT], [1], [Define if delay accounting support should be enabled.])
AM_CFLAGS="$AM_CFLAGS -I/usr/include/libnl3"
htop_config_cflags=`$PKG_CONFIG --cflags libnl-3.0 2>/dev/null`
AM_CFLAGS="$AM_CFLAGS $htop_config_cflags"
libdir_libnl=`$PKG_CONFIG --variable=libdir libnl-3.0 2>/dev/null`
Comment on lines +983 to +985
Copy link
Member

@BenBE BenBE Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failure to read these should result in a proper error message generated, or a warning notifying of a fallback to /usr/include being used.

AC_DEFINE_UNQUOTED([LIBDIR_LIBNL], ["$libdir_libnl"], [Path to the libnl-3 libraries])
Copy link
Contributor

@Explorer09 Explorer09 Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um. A small question (maybe this is bike shedding).
Which name for the shell variable / C macro is better, LIBNL_LIBDIR or LIBDIR_LIBNL?

I would vote for LIBNL_LIBDIR.

Copy link
Member

@BenBE BenBE Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other places like for hwloc use FOO_LIBS, thus LIBNL_LIBDIR would be reasonable.

fi
AM_CONDITIONAL([HAVE_DELAYACCT], [test "$enable_delayacct" = yes])

Expand Down
8 changes: 4 additions & 4 deletions linux/LibNl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,17 @@ static int load_libnl(void) {
if (libnlHandle && libnlGenlHandle)
return 0;

libnlHandle = dlopen("libnl-3.so", RTLD_LAZY);
libnlHandle = dlopen(LIBDIR_LIBNL "/libnl-3.so", RTLD_LAZY);
Copy link
Contributor

@Explorer09 Explorer09 Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a slash is used here unconditionally. Normally for libraries it's better to load them from default search paths rather than forcing a directory prefix at compile time. I know the directory is grabbed from pkg-config with this patch, but what if no directory prefix is specified there?

if (!libnlHandle) {
libnlHandle = dlopen("libnl-3.so.200", RTLD_LAZY);
libnlHandle = dlopen(LIBDIR_LIBNL "/libnl-3.so.200", RTLD_LAZY);
if (!libnlHandle) {
goto dlfailure;
}
}

libnlGenlHandle = dlopen("libnl-genl-3.so", RTLD_LAZY);
libnlGenlHandle = dlopen(LIBDIR_LIBNL "/libnl-genl-3.so", RTLD_LAZY);
if (!libnlGenlHandle) {
libnlGenlHandle = dlopen("libnl-genl-3.so.200", RTLD_LAZY);
libnlGenlHandle = dlopen(LIBDIR_LIBNL "/libnl-genl-3.so.200", RTLD_LAZY);
if (!libnlGenlHandle) {
goto dlfailure;
}
Expand Down