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

[Dave Reisner]

On Sun, Nov 20, 2011 at 01:07:14PM -0500, Andrew Gregory wrote:
> 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.

This is an DB inconsistency that we should not account for. Removal of a
package (rightfully) doesn't check ownership by other packages, and will
unconditionally remove every file in the filelist. We do not encourage
usage of -f, and in fact the flag no longer exists -- only the longopt
of '--force' remains. See 0d2600c5750.

A tool to find multiple packages owning the same file would be^W^W is
fairly straightforward to write: http://sprunge.us/XiGN. It might also
be something to consider working into testdb since it already looks for
databases which are broken in other ways...

d

> 
> > >  			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  
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://mailman.archlinux.org/pipermail/pacman-dev/attachments/20111120/9b125ef9/attachment-0001.asc>