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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211007160202.GG32194@orbyte.nwl.cc>
Date:   Thu, 7 Oct 2021 18:02:02 +0200
From:   Phil Sutter <phil@....cc>
To:     Andrea Claudi <aclaudi@...hat.com>
Cc:     netdev@...r.kernel.org, stephen@...workplumber.org,
        dsahern@...il.com, bluca@...ian.org, haliu@...hat.com
Subject: Re: [PATCH iproute2 v4 0/5] configure: add support for libdir and
 prefix option

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