[pacman-dev] [PATCH 2/6] Don't do eval

[Dave Reisner]

On Fri, Sep 30, 2011 at 01:14:02AM +0800, lolilolicon wrote:
> Simply `eval'ing configurations from pacman.conf is really bad practice.
> Use proper string parsing instead.  The parser implemented here aborts
> as soon as it gets the first assignment, being consistent with pacman.

I'm not really a fan of using awk, here, either -- totally unreadable.
We've been bitten by numerous portability issues with awk in the past.
I recommend testing against mawk and nawk if you're ever going to use
awk in our codebase. Also to note bad practice: embedding shell
variables in the awk expressions. You should be defining the variable
within the program with the -v flag to awk.

See how we do this in scripts/pacman-key in pure bash (func is
get_from() -- yes, its a terrible name). Given how frequently we seem to
do this, it might be a candidate for pulling out and maintaining a
single implementation in the scripts/library dir.

d

> Signed-off-by: lolilolicon <lolilolicon at gmail.com>
> ---
>  contrib/bacman.in               |   14 +++++++++++++-
>  contrib/pacscripts.in           |   16 ++++++++++++++--
>  scripts/pacman-db-upgrade.sh.in |   14 +++++++++++++-
>  scripts/pacman-optimize.sh.in   |   14 +++++++++++++-
>  4 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/contrib/bacman.in b/contrib/bacman.in
> index b51a4a3..0069d1e 100755
> --- a/contrib/bacman.in
> +++ b/contrib/bacman.in
> @@ -75,7 +75,19 @@ if [ ! -r @sysconfdir@/pacman.conf ]; then
>      exit 1
>  fi
>  
> -eval $(awk '/DBPath/ {print $1$2$3}' @sysconfdir@/pacman.conf)
> +# parse value of simple, non-repeating variable assignment
> +conf_var_val() {
> +	local var=${1//\//\\/}
> +	awk '
> +	/^[ \t]*'"${var}"'[ \t]*=/ {
> +		sub(/[^=]+=[ \t]*/, "")
> +		sub(/[ \t]*$/, "")
> +		print
> +		exit
> +	}'
> +}
> +
> +DBPath=$(conf_var_val DBPath < @sysconfdir@/pacman.conf)
>  pac_db="${DBPath:- at localstatedir@/lib/pacman/}/local"
>  
>  if [ ! -r @sysconfdir@/makepkg.conf ]; then
> diff --git a/contrib/pacscripts.in b/contrib/pacscripts.in
> index 37d3fea..0ac64a2 100755
> --- a/contrib/pacscripts.in
> +++ b/contrib/pacscripts.in
> @@ -34,8 +34,20 @@ if [ ! -r "$conf" ]; then
>  	exit 1
>  fi
>  
> -eval $(awk '/DBPath/ {print $1$2$3}' "$conf")
> -eval $(awk '/CacheDir/ {print $1$2$3}' "$conf")
> +# parse value of simple, non-repeating variable assignment
> +conf_var_val() {
> +	local var=${1//\//\\/}
> +	awk '
> +	/^[ \t]*'"${var}"'[ \t]*=/ {
> +		sub(/[^=]+=[ \t]*/, "")
> +		sub(/[ \t]*$/, "")
> +		print
> +		exit
> +	}'
> +}
> +
> +DBPath=$(conf_var_val DBPath < "$conf")
> +CacheDir=$(conf_var_val CacheDir < "$conf")
>  pac_db="${DBPath:- at localstatedir@/lib/pacman}/local"
>  pac_cache="${CacheDir:- at localstatedir@/cache/pacman/pkg}"
>  
> diff --git a/scripts/pacman-db-upgrade.sh.in b/scripts/pacman-db-upgrade.sh.in
> index 3e0d702..eb427ed 100644
> --- a/scripts/pacman-db-upgrade.sh.in
> +++ b/scripts/pacman-db-upgrade.sh.in
> @@ -25,7 +25,19 @@ export TEXTDOMAINDIR='@localedir@'
>  
>  myver='@PACKAGE_VERSION@'
>  
> -eval $(awk '/DBPath/ {print $1$2$3}' @sysconfdir@/pacman.conf)
> +# parse value of simple, non-repeating variable assignment
> +conf_var_val() {
> +	local var=${1//\//\\/}
> +	awk '
> +	/^[ \t]*'"${var}"'[ \t]*=/ {
> +		sub(/[^=]+=[ \t]*/, "")
> +		sub(/[ \t]*$/, "")
> +		print
> +		exit
> +	}'
> +}
> +
> +DBPath=$(conf_var_val DBPath < @sysconfdir@/pacman.conf)
>  dbroot="${DBPath:- at localstatedir@/lib/pacman/}"
>  
>  m4_include(library/output_format.sh)
> diff --git a/scripts/pacman-optimize.sh.in b/scripts/pacman-optimize.sh.in
> index 5ff302e..8e2a8bb 100644
> --- a/scripts/pacman-optimize.sh.in
> +++ b/scripts/pacman-optimize.sh.in
> @@ -26,7 +26,19 @@ export TEXTDOMAINDIR='@localedir@'
>  
>  myver='@PACKAGE_VERSION@'
>  
> -eval $(awk '/DBPath/ {print $1$2$3}' @sysconfdir@/pacman.conf)
> +# parse value of simple, non-repeating variable assignment
> +conf_var_val() {
> +	local var=${1//\//\\/}
> +	awk '
> +	/^[ \t]*'"${var}"'[ \t]*=/ {
> +		sub(/[^=]+=[ \t]*/, "")
> +		sub(/[ \t]*$/, "")
> +		print
> +		exit
> +	}'
> +}
> +
> +DBPath=$(conf_var_val DBPath < @sysconfdir@/pacman.conf)
>  dbroot="${DBPath:- at localstatedir@/lib/pacman/}"
>  
>  m4_include(library/output_format.sh)
> -- 
> 1.7.6.4
> 
>