On Sun, 20 Nov 2011 12:39:11 -0500
Dave Reisner <d...@falconindy.com> wrote:
> On Sun, Nov 20, 2011 at 12:18:59PM -0500, andrew.gregor...@gmail.com
> wrote:
> > From: Andrew Gregory <andrew.gregor...@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.gregor...@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
