[PATCH iproute2 v4 0/5] configure: add support for libdir and prefix option

This series add support for the libdir parameter in iproute2 configure
system. The idea is to make use of the fact that packaging systems may
assume that 'configure' comes from autotools allowing a syntax similar
to the autotools one, and using it to tell iproute2 where the distro
expects to find its lib files.

Patches 1-2 fix a parsing issue on current configure options, that may
trigger an endless loop when no value is provided with some options;

Patch 3 introduces support for the --opt=value style on current options,
for uniformity;

Patch 4 add the --prefix option, that may be used by some packaging
systems when calling the configure script;

Patch 5 add the --libdir option, and also drops the static LIBDIR var
from the Makefile

Changelog:
----------
v3 -> v4
  - fix parsing issue on '--include_dir' and '--libbpf_dir'
  - split '--opt value' and '--opt=value' use cases, avoid code
    duplication moving semantic checks on value to dedicated functions

v2 -> v3
  - fix parsing error on prefix and libdir options.

v1 -> v2
  - consolidate '--opt value' and '--opt=value' use cases, as suggested
    by David Ahern.
  - added patch 2 to manage the --prefix option, used by the Debian
    packaging system, as reported by Luca Boccassi, and use it when
    setting lib directory.

Andrea Claudi (5):
  configure: fix parsing issue on include_dir option
  configure: fix parsing issue on libbpf_dir option
  configure: support --param=value style
  configure: add the --prefix option
  configure: add the --libdir option

 Makefile  |  7 +++--
 configure | 85 +++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 80 insertions(+), 12 deletions(-)

-- 
2.31.1

configure is stuck in an endless loop if '--include_dir' option is used
without a value:

$ ./configure --include_dir
./configure: line 506: shift: 2: shift count out of range
./configure: line 506: shift: 2: shift count out of range
[...]

Fix it checking that a value is provided with the option.
A dedicated function is used to avoid code duplication, as this check will
be needed for further options that may be introduced in the future.

Fixes: a9c3d70d902a ("configure: add options ability")
Signed-off-by: Andrea Claudi <redacted>
---
 configure | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 7f4f3bd9..05f75437 100755
--- a/configure
+++ b/configure

@@ -485,7 +485,7 @@ usage()
 {
 	cat <<EOF
 Usage: $0 [OPTIONS]
-	--include_dir		Path to iproute2 include dir
+	--include_dir <dir>	Path to iproute2 include dir
 	--libbpf_dir		Path to libbpf DESTDIR
 	--libbpf_force		Enable/disable libbpf by force. Available options:
 				  on: require link against libbpf, quit config if no libbpf support

@@ -495,6 +495,11 @@ EOF
 	exit $1
 }
 
+check_value()
+{
+	[ -z "$1" ] && usage 1
+}
+
 # Compat with the old INCLUDE path setting method.
 if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then
 	INCLUDE="$1"

@@ -503,6 +508,7 @@ else
 		case "$1" in
 			--include_dir)
 				INCLUDE=$2
+				check_value "$INCLUDE"
 				shift 2 ;;
 			--libbpf_dir)
 				LIBBPF_DIR="$2"

-- 
2.31.1

configure is stuck in an endless loop if '--libbpf_dir' option is used
without a value:

$ ./configure --libbpf_dir
./configure: line 515: shift: 2: shift count out of range
./configure: line 515: shift: 2: shift count out of range
[...]

Fix it checking that a value is provided with the option.

Fixes: 7ae2585b865a ("configure: convert LIBBPF environment variables to command-line options")
Signed-off-by: Andrea Claudi <redacted>
---
 configure | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 05f75437..27db3ecb 100755
--- a/configure
+++ b/configure

@@ -486,7 +486,7 @@ usage()
 	cat <<EOF
 Usage: $0 [OPTIONS]
 	--include_dir <dir>	Path to iproute2 include dir
-	--libbpf_dir		Path to libbpf DESTDIR
+	--libbpf_dir <dir>	Path to libbpf DESTDIR
 	--libbpf_force		Enable/disable libbpf by force. Available options:
 				  on: require link against libbpf, quit config if no libbpf support
 				  off: disable libbpf probing

@@ -512,6 +512,7 @@ else
 				shift 2 ;;
 			--libbpf_dir)
 				LIBBPF_DIR="$2"
+				check_value "$LIBBPF_DIR"
 				shift 2 ;;
 			--libbpf_force)
 				if [ "$2" != 'on' ] && [ "$2" != 'off' ]; then

-- 
2.31.1

This commit makes it possible to specify values for configure params
using the common autotools configure syntax '--param=value'.

To avoid code duplication, semantic check on libbpf_force is moved to a
dedicated function.

Signed-off-by: Andrea Claudi <redacted>
---
 configure | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index 27db3ecb..08e7410b 100755
--- a/configure
+++ b/configure

@@ -485,12 +485,12 @@ usage()
 {
 	cat <<EOF
 Usage: $0 [OPTIONS]
-	--include_dir <dir>	Path to iproute2 include dir
-	--libbpf_dir <dir>	Path to libbpf DESTDIR
-	--libbpf_force		Enable/disable libbpf by force. Available options:
-				  on: require link against libbpf, quit config if no libbpf support
-				  off: disable libbpf probing
-	-h | --help		Show this usage info
+	--include_dir <dir>		Path to iproute2 include dir
+	--libbpf_dir <dir>		Path to libbpf DESTDIR
+	--libbpf_force <on|off>		Enable/disable libbpf by force.
+					  on: require link against libbpf, quit config if no libbpf support
+					  off: disable libbpf probing
+	-h | --help			Show this usage info
 EOF
 	exit $1
 }

@@ -500,6 +500,13 @@ check_value()
 	[ -z "$1" ] && usage 1
 }
 
+check_onoff()
+{
+	if [ "$1" != 'on' ] && [ "$1" != 'off' ]; then
+		usage 1
+	fi
+}
+
 # Compat with the old INCLUDE path setting method.
 if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then
 	INCLUDE="$1"

@@ -510,16 +517,26 @@ else
 				INCLUDE=$2
 				check_value "$INCLUDE"
 				shift 2 ;;
+			--include_dir=*)
+				INCLUDE="${1#*=}"
+				check_value "$INCLUDE"
+				shift ;;
 			--libbpf_dir)
 				LIBBPF_DIR="$2"
 				check_value "$LIBBPF_DIR"
 				shift 2 ;;
+			--libbpf_dir=*)
+				LIBBPF_DIR="${1#*=}"
+				check_value "$LIBBPF_DIR"
+				shift ;;
 			--libbpf_force)
-				if [ "$2" != 'on' ] && [ "$2" != 'off' ]; then
-					usage 1
-				fi
 				LIBBPF_FORCE=$2
+				check_onoff "$LIBBPF_FORCE"
 				shift 2 ;;
+			--libbpf_force=*)
+				LIBBPF_FORCE="${1#*=}"
+				check_onoff "$LIBBPF_FORCE"
+				shift ;;
 			-h | --help)
 				usage 0 ;;
 			"")

@@ -528,6 +545,7 @@ else
 				shift 1 ;;
 		esac
 	done
+
 fi
 
 echo "# Generated config based on" $INCLUDE >$CONFIG

-- 
2.31.1

This commit add the '--prefix' option to the iproute2 configure script.

This mimics the '--prefix' option that autotools configure provides, and
will be used later to allow users or packagers to set the lib directory.

Signed-off-by: Andrea Claudi <redacted>
---
 configure | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/configure b/configure
index 08e7410b..2e5ed87e 100755
--- a/configure
+++ b/configure

@@ -148,6 +148,15 @@ EOF
 	rm -f $TMPDIR/ipttest.c $TMPDIR/ipttest
 }
 
+check_prefix()
+{
+	if [ -n "$PREFIX" ]; then
+		prefix="$PREFIX"
+	else
+		prefix="/usr"
+	fi
+}
+
 check_ipt()
 {
 	if ! grep TC_CONFIG_XT $CONFIG > /dev/null; then

@@ -490,6 +499,7 @@ Usage: $0 [OPTIONS]
 	--libbpf_force <on|off>		Enable/disable libbpf by force.
 					  on: require link against libbpf, quit config if no libbpf support
 					  off: disable libbpf probing
+	--prefix <dir>			Path prefix of the lib files to install
 	-h | --help			Show this usage info
 EOF
 	exit $1

@@ -537,6 +547,14 @@ else
 				LIBBPF_FORCE="${1#*=}"
 				check_onoff "$LIBBPF_FORCE"
 				shift ;;
+			--prefix)
+				PREFIX="$2"
+				check_value "$PREFIX"
+				shift 2 ;;
+			--prefix=*)
+				PREFIX="${1#*=}"
+				check_value "$PREFIX"
+				shift ;;
 			-h | --help)
 				usage 0 ;;
 			"")

@@ -571,6 +589,7 @@ if ! grep -q TC_CONFIG_NO_XT $CONFIG; then
 fi
 
 echo
+check_prefix
 if ! grep -q TC_CONFIG_NO_XT $CONFIG; then
 	echo -n "iptables modules directory: "
 	check_ipt_lib_dir

-- 
2.31.1

This commit allows users/packagers to choose a lib directory to store
iproute2 lib files.

At the moment iproute2 ship lib files in /usr/lib and offers no way to
modify this setting. However, according to the FHS, distros may choose
"one or more variants of the /lib directory on systems which support
more than one binary format" (e.g. /usr/lib64 on Fedora).

As Luca states in commit a3272b93725a ("configure: restore backward
compatibility"), packaging systems may assume that 'configure' is from
autotools, and try to pass it some parameters.

Allowing the '--libdir=/path/to/libdir' syntax, we can use this to our
advantage, and let the lib directory to be chosen by the distro
packaging system.

Signed-off-by: Andrea Claudi <redacted>
---
 Makefile  |  7 ++++---
 configure | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 5bc11477..45655ca4 100644
--- a/Makefile
+++ b/Makefile

@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 # Top level Makefile for iproute2
 
+-include config.mk
+
 ifeq ("$(origin V)", "command line")
 VERBOSE = $(V)
 endif

@@ -13,7 +15,6 @@ MAKEFLAGS += --no-print-directory
 endif
 
 PREFIX?=/usr
-LIBDIR?=$(PREFIX)/lib
 SBINDIR?=/sbin
 CONFDIR?=/etc/iproute2
 NETNS_RUN_DIR?=/var/run/netns

@@ -60,7 +61,7 @@ SUBDIRS=lib ip tc bridge misc netem genl tipc devlink rdma dcb man vdpa
 LIBNETLINK=../lib/libutil.a ../lib/libnetlink.a
 LDLIBS += $(LIBNETLINK)
 
-all: config
+all: config.mk
 	@set -e; \
 	for i in $(SUBDIRS); \
 	do echo; echo $$i; $(MAKE) -C $$i; done

@@ -80,7 +81,7 @@ help:
 	@echo "Make Arguments:"
 	@echo " V=[0|1]             - set build verbosity level"
 
-config:
+config.mk:
 	@if [ ! -f config.mk -o configure -nt config.mk ]; then \
 		sh configure $(KERNEL_INCLUDE); \
 	fi

diff --git a/configure b/configure
index 2e5ed87e..edd03467 100755
--- a/configure
+++ b/configure

@@ -157,6 +157,19 @@ check_prefix()
 	fi
 }
 
+check_lib_dir()
+{
+	echo -n "lib directory: "
+	if [ -n "$LIBDIR" ]; then
+		eval echo "$LIBDIR"
+		eval echo "LIBDIR:=$LIBDIR" >> $CONFIG
+		return
+	fi
+
+	eval echo "${prefix}/lib"
+	eval echo "LIBDIR:=${prefix}/lib" >> $CONFIG
+}
+
 check_ipt()
 {
 	if ! grep TC_CONFIG_XT $CONFIG > /dev/null; then

@@ -495,6 +508,7 @@ usage()
 	cat <<EOF
 Usage: $0 [OPTIONS]
 	--include_dir <dir>		Path to iproute2 include dir
+	--libdir <dir>			Path to iproute2 lib dir
 	--libbpf_dir <dir>		Path to libbpf DESTDIR
 	--libbpf_force <on|off>		Enable/disable libbpf by force.
 					  on: require link against libbpf, quit config if no libbpf support

@@ -531,6 +545,14 @@ else
 				INCLUDE="${1#*=}"
 				check_value "$INCLUDE"
 				shift ;;
+			--libdir)
+				LIBDIR="$2"
+				check_value "$LIBDIR"
+				shift 2 ;;
+			--libdir=*)
+				LIBDIR="${1#*=}"
+				check_value "$LIBDIR"
+				shift ;;
 			--libbpf_dir)
 				LIBBPF_DIR="$2"
 				check_value "$LIBBPF_DIR"

@@ -590,6 +612,7 @@ fi
 
 echo
 check_prefix
+check_lib_dir
 if ! grep -q TC_CONFIG_NO_XT $CONFIG; then
 	echo -n "iptables modules directory: "
 	check_ipt_lib_dir

-- 
2.31.1

Hi Andrea,

On Thu, Oct 07, 2021 at 03:40:00PM +0200, Andrea Claudi wrote:

This series add support for the libdir parameter in iproute2 configure
system. The idea is to make use of the fact that packaging systems may
assume that 'configure' comes from autotools allowing a syntax similar
to the autotools one, and using it to tell iproute2 where the distro
expects to find its lib files.

Patches 1-2 fix a parsing issue on current configure options, that may
trigger an endless loop when no value is provided with some options;

Hmm, "shift 2" is nasty. Good to be reminded that it fails if '$# < 2'.
I would avoid the loop using single shifts:

| case "$1" in
| --include_dir)
| 	shift
| 	INCLUDE=$1
| 	shift
| 	;;
| [...]

Patch 3 introduces support for the --opt=value style on current options,
for uniformity;

My idea to avoid code duplication was to move the semantic checks out of
the argument parsing loop, basically:

| [ -d "$INCLUDE" ] || usage 1
| case "$LIBBPF_FORCE" in
| 	on|off|"") ;;
| 	*) usage 1 ;;
| esac

after the loop or even before 'echo "# Generated config ...'. This
reduces the parsing loop to cases like:

| --include_dir)
| 	shift
| 	INCLUDE=$1
| 	shift
| 	;;
| --include_dir=*)
| 	INCLUDE=${1#*=}
| 	shift
| 	;;

Patch 4 add the --prefix option, that may be used by some packaging
systems when calling the configure script;

So this parses into $PREFIX and when checking it assigns to $prefix but
neither one of the two variables is used afterwards? Oh, there's patch
5 ...

Patch 5 add the --libdir option, and also drops the static LIBDIR var
from the Makefile

Can't you just:

| [ -n "$PREFIX" ] && echo "PREFIX=\"$PREFIX\"" >>config.mk
| [ -n "$LIBDIR" ] && echo "LIBDIR=\"$LIBDIR\"" >>config.mk

and leave the default ("?=") cases in Makefile in place?

Either way, calling 'eval' seems needless. I would avoid it at all
costs, "eval is evil". ;)

Cheers, Phil

On Thu, Oct 07, 2021 at 06:02:02PM +0200, Phil Sutter wrote:

Hi Andrea,

On Thu, Oct 07, 2021 at 03:40:00PM +0200, Andrea Claudi wrote:

quoted

This series add support for the libdir parameter in iproute2 configure
system. The idea is to make use of the fact that packaging systems may
assume that 'configure' comes from autotools allowing a syntax similar
to the autotools one, and using it to tell iproute2 where the distro
expects to find its lib files.

Patches 1-2 fix a parsing issue on current configure options, that may
trigger an endless loop when no value is provided with some options;

Hmm, "shift 2" is nasty. Good to be reminded that it fails if '$# < 2'.
I would avoid the loop using single shifts:

| case "$1" in
| --include_dir)
| 	shift
| 	INCLUDE=$1
| 	shift
| 	;;
| [...]

This avoid the endless loop and allows configure to terminate correctly,
but results in an error anyway:

$ ./configure --include_dir
./configure: line 544: shift: shift count out of range

But thanks anyway! Your comment made me think again about this, and I
think we can use the *) case to actually get rid of the second shift.

Indeed, when an option is specified, the --opt case will shift and get
its value, then the next while loop will take the *) case, and the
second shift is triggered this way.

quoted

Patch 3 introduces support for the --opt=value style on current options,
for uniformity;

My idea to avoid code duplication was to move the semantic checks out of
the argument parsing loop, basically:

| [ -d "$INCLUDE" ] || usage 1
| case "$LIBBPF_FORCE" in
| 	on|off|"") ;;
| 	*) usage 1 ;;
| esac

after the loop or even before 'echo "# Generated config ...'. This
reduces the parsing loop to cases like:

| --include_dir)
| 	shift
| 	INCLUDE=$1
| 	shift
| 	;;
| --include_dir=*)
| 	INCLUDE=${1#*=}
| 	shift
| 	;;

Thanks. I didn't think about '-d', this also cover corner cases like:

$ ./configure --include_dir --libbpf_force off

that results in INCLUDE="--libbpf_force".

quoted

Patch 4 add the --prefix option, that may be used by some packaging
systems when calling the configure script;

So this parses into $PREFIX and when checking it assigns to $prefix but
neither one of the two variables is used afterwards? Oh, there's patch
5 ...

quoted

Patch 5 add the --libdir option, and also drops the static LIBDIR var
from the Makefile

Can't you just:

| [ -n "$PREFIX" ] && echo "PREFIX=\"$PREFIX\"" >>config.mk
| [ -n "$LIBDIR" ] && echo "LIBDIR=\"$LIBDIR\"" >>config.mk

and leave the default ("?=") cases in Makefile in place?

Either way, calling 'eval' seems needless. I would avoid it at all
costs, "eval is evil". ;)

Unfortunately this is needed because some packaging systems uses
${prefix} as an argument to --libdir, expecting this to be replaced with
the value of --prefix. See Luca's review to v1 for an example [1].

I can always avoid the eval trying to parse "${prefix}" and replacing it
with the PREFIX value, but in this case "eval" seems a bit more
practical to me... WDYT?

Regards,
Andrea

[1] https://lore.kernel.org/netdev/6363502d3ce806acdbc7ba194ddc98d3fac064de.camel@debian.org/

Hi,

On Fri, Oct 08, 2021 at 03:08:23PM +0200, Andrea Claudi wrote:

On Thu, Oct 07, 2021 at 06:02:02PM +0200, Phil Sutter wrote:

quoted

On Thu, Oct 07, 2021 at 03:40:00PM +0200, Andrea Claudi wrote:

quoted

This series add support for the libdir parameter in iproute2 configure
system. The idea is to make use of the fact that packaging systems may
assume that 'configure' comes from autotools allowing a syntax similar
to the autotools one, and using it to tell iproute2 where the distro
expects to find its lib files.

Patches 1-2 fix a parsing issue on current configure options, that may
trigger an endless loop when no value is provided with some options;

Hmm, "shift 2" is nasty. Good to be reminded that it fails if '$# < 2'.
I would avoid the loop using single shifts:

| case "$1" in
| --include_dir)
| 	shift
| 	INCLUDE=$1
| 	shift
| 	;;
| [...]

This avoid the endless loop and allows configure to terminate correctly,
but results in an error anyway:

$ ./configure --include_dir
./configure: line 544: shift: shift count out of range

Ah, I didn't see it with bash. I don't think it's a problem though:
Input is invalid, the loop is avoided and (depending on your patches)
there will be another error message complaining about invalid $INCLUDE
value.

But thanks anyway! Your comment made me think again about this, and I
think we can use the *) case to actually get rid of the second shift.

Indeed, when an option is specified, the --opt case will shift and get
its value, then the next while loop will take the *) case, and the
second shift is triggered this way.

Which sounds like you'll start accepting things like

| ./configure --include_dir foo bar

quoted

quoted

Patch 3 introduces support for the --opt=value style on current options,
for uniformity;

My idea to avoid code duplication was to move the semantic checks out of
the argument parsing loop, basically:

| [ -d "$INCLUDE" ] || usage 1
| case "$LIBBPF_FORCE" in
| 	on|off|"") ;;
| 	*) usage 1 ;;
| esac

after the loop or even before 'echo "# Generated config ...'. This
reduces the parsing loop to cases like:

| --include_dir)
| 	shift
| 	INCLUDE=$1
| 	shift
| 	;;
| --include_dir=*)
| 	INCLUDE=${1#*=}
| 	shift
| 	;;

Thanks. I didn't think about '-d', this also cover corner cases like:

$ ./configure --include_dir --libbpf_force off

that results in INCLUDE="--libbpf_force".

A common case would be (note the typo):

| ./configure --include_dir $MY_INCULDE_DIR --libbpf_force off

quoted

quoted

Patch 4 add the --prefix option, that may be used by some packaging
systems when calling the configure script;

So this parses into $PREFIX and when checking it assigns to $prefix but
neither one of the two variables is used afterwards? Oh, there's patch
5 ...

quoted

Patch 5 add the --libdir option, and also drops the static LIBDIR var
from the Makefile

Can't you just:

| [ -n "$PREFIX" ] && echo "PREFIX=\"$PREFIX\"" >>config.mk
| [ -n "$LIBDIR" ] && echo "LIBDIR=\"$LIBDIR\"" >>config.mk

and leave the default ("?=") cases in Makefile in place?

Either way, calling 'eval' seems needless. I would avoid it at all
costs, "eval is evil". ;)

Unfortunately this is needed because some packaging systems uses
${prefix} as an argument to --libdir, expecting this to be replaced with
the value of --prefix. See Luca's review to v1 for an example [1].

I can always avoid the eval trying to parse "${prefix}" and replacing it
with the PREFIX value, but in this case "eval" seems a bit more
practical to me... WDYT?

Do autotools support that? If not, I wouldn't bother.

Cheers, Phil

On Fri, Oct 08, 2021 at 03:50:25PM +0200, Phil Sutter wrote:

[...]

quoted

This avoid the endless loop and allows configure to terminate correctly,
but results in an error anyway:

$ ./configure --include_dir
./configure: line 544: shift: shift count out of range

Ah, I didn't see it with bash. I don't think it's a problem though:
Input is invalid, the loop is avoided and (depending on your patches)
there will be another error message complaining about invalid $INCLUDE
value.

Yes, this error can be disregarded. Still I would try to avoid a
meaningless error message, if possible.

[...]

Which sounds like you'll start accepting things like

| ./configure --include_dir foo bar

We already accept things like this in the current configure, and I would
try to not modify current behaviour as much as possible.

[...]

quoted

quoted

Can't you just:

| [ -n "$PREFIX" ] && echo "PREFIX=\"$PREFIX\"" >>config.mk
| [ -n "$LIBDIR" ] && echo "LIBDIR=\"$LIBDIR\"" >>config.mk

and leave the default ("?=") cases in Makefile in place?

Either way, calling 'eval' seems needless. I would avoid it at all
costs, "eval is evil". ;)

Unfortunately this is needed because some packaging systems uses
${prefix} as an argument to --libdir, expecting this to be replaced with
the value of --prefix. See Luca's review to v1 for an example [1].

I can always avoid the eval trying to parse "${prefix}" and replacing it
with the PREFIX value, but in this case "eval" seems a bit more
practical to me... WDYT?

Do autotools support that? If not, I wouldn't bother.

I don't know about autotools, but Debian packaging system makes use of
this, and we cannot break their workflow.

Regards,
Andrea

On 10/8/21 10:19 AM, Andrea Claudi wrote:

quoted

Which sounds like you'll start accepting things like

| ./configure --include_dir foo bar

We already accept things like this in the current configure, and I would
try to not modify current behaviour as much as possible.

that is definitely not intended and I don't see a reason to allow it.