[pacman-dev] [PATCH] allow querying fileowner for directories

[Andrew Gregory]

On Sun, 20 Nov 2011 12:39:11 -0500
Dave Reisner <d at falconindy.com> wrote:

> On Sun, Nov 20, 2011 at 12:18:59PM -0500, andrew.gregory.8 at gmail.com
> wrote:  
> > From: Andrew Gregory <andrew.gregory.8 at gmail.com>
> > 
> > Not allowing fileowner queries for directories was an unnecessary
> > limitation.  Queries for directories have poor performance due to
> > having to call real_path on every directory listed in every
> > package's file list.  Because more than one package will likely own
> > a given directory, all packages are now checked instead of bailing
> > out after the first owner is found.
> > 
> > Signed-off-by: Andrew Gregory <andrew.gregory.8 at gmail.com>
> > ---
> >  src/pacman/query.c |   17 +++++++++--------
> >  1 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/pacman/query.c b/src/pacman/query.c
> > index 4c2ea81..8162662 100644
> > --- a/src/pacman/query.c
> > +++ b/src/pacman/query.c
> > @@ -127,7 +127,7 @@ static int query_fileowner(alpm_list_t *targets)
> >  	 * iteration. */
> >  	root = alpm_option_get_root(config->handle);
> >  	rootlen = strlen(root);
> > -	if(rootlen + 1 > PATH_MAX) {
> > +	if(rootlen >= PATH_MAX) {
> >  		/* we are in trouble here */
> >  		pm_printf(ALPM_LOG_ERROR, _("path too long:
> > %s%s\n"), root, ""); return 1;
> > @@ -164,12 +164,13 @@ static int query_fileowner(alpm_list_t
> > *targets) }
> >  		}
> >  
> > +		/* make sure directories end with '/' */
> >  		if(S_ISDIR(buf.st_mode)) {
> > -			pm_printf(ALPM_LOG_ERROR,
> > -				_("cannot determine ownership of
> > directory '%s'\n"), filename);
> > -			ret++;
> > -			free(filename);
> > -			continue;
> > +			int fnlen = strlen(filename);  
> 
> strlen returns a size_t, not an int. Even though the compiler won't
> complain, we're pretty good about adhering to type correctness in the
> codebase.
>   

will fix

> > +			if(filename[fnlen-1] != '/'){
> > +				filename = realloc(filename,
> > sizeof(*filename) * (fnlen+2));  
> 
> We use sizeof(type) rather than sizeof(*ptr).
>   

The HACKING file mentions, and I personally agree, that sizeof(*ptr) may
be better.  Perhaps we should switch to sizeof(*ptr) or update the
HACKING file.

> > +				strcat(filename, "/");
> > +			}
> >  		}
> >  
> >  		bname = mbasename(filename);
> > @@ -192,7 +193,7 @@ static int query_fileowner(alpm_list_t *targets)
> >  		}
> >  		free(dname);
> >  
> > -		for(i = alpm_db_get_pkgcache(db_local); i
> > && !found; i = alpm_list_next(i)) {
> > +		for(i = alpm_db_get_pkgcache(db_local); i ; i =
> > alpm_list_next(i)) {  
> 
> This hurts performance of _all_ -Qo queries, which I'm not a fan of.
> Why not flag the query as needing to continue through the entire
> local DB specifically for directory queries?
>   

I considered that, but given that use of the -f flag could lead to more
than one package owning a file, I thought it would be best to check all
packages for files as well.

> >  			alpm_pkg_t *info = i->data;
> >  			alpm_filelist_t *filelist =
> > alpm_pkg_get_files(info); size_t j;
> > @@ -214,7 +215,7 @@ static int query_fileowner(alpm_list_t *targets)
> >  					continue;
> >  				}
> >  
> > -				if(rootlen + 1 + strlen(pkgfile) >
> > PATH_MAX) {
> > +				if(rootlen + strlen(pkgfile) >=
> > PATH_MAX) { pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"),
> > root, pkgfile); }
> >  				/* concatenate our file and the
> > root path */ -- 
> > 1.7.7.3
> >   
> 
> dave