[pacman-dev] [PATCH 1/3] pacdiff: improve speed, accuracy finding active configs using pacmandb

[Dave Reisner]

On Thu, Jul 18, 2013 at 06:17:32PM -0400, Jonathan Frazier wrote:
> This is a new search type, using -p or --pacmandb options. It reads
> config file locations directly from the local pacman db. It will find
> active configs anywhere they are defined in installed packages. It is
> not dependant on outside configs such as updatedb.conf or scanning a
> large set of directories for find.
> 
> This will find more pacnews than find when searching with the current
> default of /etc, and it is faster than both find and updatedb when
> searching the entire fs. When run directly after an update, the local db
> is more likely to be cached than all files in /etc or / as other methods
> read. This will increase performance further post upgrade.
> 
> After a package is removed and a pacsave is created, this method will
> not find these pacsaves until the base config is added to the local db
> again. These files have no influence in a working system and only take
> up a few blocks of disk space.
> 
> Active configs need to be dealt with immediately to keep a system
> working. pacsaves related to removed configs can remain for weeks or
> months without problems. I would recommend occasionally running other
> methods such as --locate to remove them.
> 
> Signed-off-by: Jonathan Frazier <eyeswide at gmail.com>
> ---
>  contrib/pacdiff.sh.in | 46 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/pacdiff.sh.in b/contrib/pacdiff.sh.in
> index 7a2db2c..60c7c63 100644
> --- a/contrib/pacdiff.sh.in
> +++ b/contrib/pacdiff.sh.in
> @@ -25,7 +25,7 @@ diffprog=${DIFFPROG:-vimdiff}
>  diffsearchpath=${DIFFSEARCHPATH:-/etc}
>  USE_COLOR='y'
>  
> -declare -i findActive=0 locateActive=0
> +declare -i findActive=0 locateActive=0 dbActive=0
>  declare -a oldsaves
>  
>  m4_include(../scripts/library/output_format.sh)
> @@ -34,11 +34,12 @@ usage() {
>  	cat <<EOF
>  $myname is a simple pacnew/pacorig/pacsave updater.
>  
> -Usage: $myname [-l | -f] [--nocolor]
> +Usage: $myname [-l | -f | -p] [--nocolor]
>  
>  Search Options:     select one, default: find
>    -l/--locate       scan using locate
>    -f/--find         scan using find
> +  -p/--pacmandb     scan active config files from pacman db
>  
>  General Options:
>    --nocolor         remove colors from output
> @@ -59,11 +60,32 @@ version() {
>  	echo 'Copyright (C) 2013 Pacman Development Team <pacman-dev at archlinux.org>'
>  }
>  
> +check_backup() {
> +	[[ -f "$1" ]] && printf "$1"'\0'

If this were C, this would be a format string injection vulnerability.

  [[ -f $1 ]] && printf '%s\0' "$1"

Quotes are not needed for single argument bash tests. And, I'd
recommend changing the naem of this function since the point of it isn't
really to "check" anything, but print the filename null delimited.

> +}
> +
> +check_pacsave(){
> +	for f in "${1}" ${1}.[0-9]; do

Strange that you quoted one of these but not the other. You're hitting
the same 10+ pacsave problem here, too.

  for f in "$1"{,.+([0-9])}

> +		check_backup "$f"
> +	done
> +}	
> +
>  cmd() {
>  	if (( $locateActive )); then
>  		locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave '*.pacsave.[0-9]'
>  	elif (( $findActive )); then
>  		find $diffsearchpath \( -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -o -name '*.pacsave.[0-9]' \) -print0
> +	elif (( $dbActive )); then

I realize you're just copying what's already been done, but please don't
forcibly expand vars in a math context, especially if they're declared
with the -i attribute. Just (( dbActive )) is fine.

> +		awk '/^%BACKUP%$/ {
> +		while (getline) {
> +			if (/^$/) { nextfile }
> +				print $1
> +			}
> +		}' "${pac_db}"/*/files | while read -r bkup; do
> +			check_backup "/$bkup.pacnew"
> +			check_backup "/$bkup.pacorig"
> +			check_pacsave "/$bkup.pacsave"
> +		done
>  	fi
>  }
>  
> @@ -73,8 +95,10 @@ while [[ -n "$1" ]]; do
>  		locateActive=1;;
>  		-f|--find)
>  		findActive=1;;
> +		-p|--pacmandb)
> +		dbActive=1;;
>  		--nocolor)
> -		USE_COLOR='n' ;;
> +		USE_COLOR='n';;
>  		-V|--version)
>  		version; exit 0;;
>  		-h|--help)
> @@ -87,12 +111,26 @@ done
>  
>  m4_include(../scripts/library/term_colors.sh)
>  
> -case $(( findActive+locateActive )) in
> +case $(( findActive+locateActive+dbActive )) in
>  	0) findActive=1;; # set the default search option
>  	[^1]) error "Only one search option may be used at a time"
>  	 	usage; exit 1;;
>  esac
>  
> +if (( dbActive )); then
> +	if [[ ! -r @sysconfdir@/pacman.conf ]]; then
> +		error "unable to read @sysconfdir@/pacman.conf"
> +		usage; exit 1
> +	fi
> +
> +	eval $(awk '/DBPath/ {print $1$2$3}' @sysconfdir@/pacman.conf)

I wish this expression wasn't so common in our scripts... it's so
wrought with problems I don't even know where to begin.

> +	pac_db="${DBPath:- at localstatedir@/lib/pacman/}local"
> +	if [[ ! -d "${pac_db}" ]]; then
> +		error "unable to read pacman db %s". "${pac_db}"
> +		usage; exit 1
> +	fi
> +fi
> +
>  # see http://mywiki.wooledge.org/BashFAQ/020
>  while IFS= read -u 3 -r -d '' pacfile; do
>  	file="${pacfile%.pac*}"
> -- 
> 1.8.3.2
> 
>