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@falconindy.com> wrote:

...

On Sun, Nov 20, 2011 at 12:18:59PM -0500, andrew.gregory.8@gmail.com wrote:

...

From: Andrew Gregory <andrew.gregory.8@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@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

On Sun, Nov 20, 2011 at 3:03 PM, <andrew.gregory.8@gmail.com> wrote:

From: Andrew Gregory <andrew.gregory.8@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 checked when querying a directory instead of bailing out after the first owner is found.

Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---  src/pacman/query.c |   21 ++++++++++++---------  1 files changed, 12 insertions(+), 9 deletions(-) Sorry I didn't get a chance to look at this earlier.

First, I should say I've tried to do this 5 months ago, but didn't make my patch public at the time. It is now here: http://code.toofishes.net/cgit/dan/pacman.git/commit/?h=dir-ownership I'll end up calling out things in there I had to do, as unfortunately there are a bazillion edge cases I encountered when writing the patch.

diff --git a/src/pacman/query.c b/src/pacman/query.c index 4c2ea81..c64f0d2 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; @@ -142,6 +142,7 @@ static int query_fileowner(alpm_list_t *targets)                struct stat buf;                alpm_list_t *i;                int found = 0; +               int searchall = 0;

               filename = strdup(t->data);

@@ -164,12 +165,14 @@ 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; +                       searchall = 1; +                       size_t fnlen = strlen(filename); +                       if(filename[fnlen-1] != '/'){ +                               filename = realloc(filename, sizeof(char) * (fnlen+2)); sizeof(char) is always 1 guaranteed, no need for that. +                               strcat(filename, "/"); strcat is rather inefficient when you've already got yourself the total length of the string; you can simply set filename[len - 1] = '/', filename[len] = '\0' in this case or whatever it needs to be. +                       }                } I took the opposite approach here: ensured it did NOT end with a slash. This vastly simplifies the memory hoops you had to jump through here.

               bname = mbasename(filename); @@ -192,7 +195,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 && (searchall || !found); i = alpm_list_next(i)) {

This looks so eerily similar...

                       alpm_pkg_t *info = i->data;                        alpm_filelist_t *filelist = alpm_pkg_get_files(info);                        size_t j; @@ -211,10 +214,10 @@ static int query_fileowner(alpm_list_t *targets) And now this is where we differ.

Things I came across that occur in the latter half of my patch: * We can break once we've seen a directory in a package; however there are two print_query_fileowner() calls that need to be handled; you only handled one of them. * I added code to (cheaply) bail early on a given file if we knew we were looking for a directory; this saves a lot of expense calling mdirname/resolve_path/etc. unnecessarily. * I want to say the reason I stripped the slash had to do with all sorts of edge cases involving symlinks. I don't know this for sure though. I'd check your code on something like the following: $ ln -s /usr/share /tmp/foobar $ ./src/pacman/pacman -Qo /usr/share/ | wc -l 739 $ ./src/pacman/pacman -Qo /usr/share | wc -l 739 $ ./src/pacman/pacman -Qo /tmp/foobar/ | wc -l error: No package owns /tmp/foobar 0 $ ./src/pacman/pacman -Qo /tmp/foobar | wc -l error: No package owns /tmp/foobar 0 $ ./src/pacman/pacman -Qo /var/mail /var/mail/ /var/spool/mail /var/spool/mail/ /var/mail is owned by filesystem 2011.10-1 /var/mail is owned by filesystem 2011.10-1 /var/spool/mail is owned by filesystem 2011.10-1 /var/spool/mail is owned by filesystem 2011.10-1 Sorry this was hidden away; wish I could have saved you some duplicate work. I encourage you to combine the work though and resubmit after thorough testing.

                               if(!rpath) {                                        print_query_fileowner(filename, info);                                        found = 1; -                                       continue; +                                       break;                                }

-                               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

On Tue, Nov 22, 2011 at 10:57 AM, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:

Think I've got it all worked out now.  Replaced mdirname and mbasename with posix equivalent (but safer) functions.  Directories no longer need to be treated any differently than files other than setting searchall.  I'd like to suggest one more time that we go ahead and search all packages for files too even though more than one package *shouldn't* own a file.  Otherwise, here's an updated patch. Half of this is a commit message, half is commentary. Git provides an easy way to address this- if you simply write your comments where I indicate below, they won't be included when I git-am the patch. Can you resubmit with a commit message that doesn't depend on context that isn't there in the repository in a year, please?

(the test results are definitely fine; but the "updated patch", "worked out now", etc. bits are all unnecessary)

Test Results: ln -s /usr/share /tmp/foo /usr/local/bin/pacman -Qo /var/spool/mail /var/spool/mail/ /var/mail  /var/mail/ /tmp/foo /tmp/foo/ firefox /usr/bin/firefox / /nonexistent

/var/spool/mail is owned by filesystem 2011.10-1 /var/spool/mail/ is owned by filesystem 2011.10-1 /var/mail is owned by filesystem 2011.10-1 /var/mail/ is owned by filesystem 2011.10-1 error: No package owns /tmp/foo error: No package owns /tmp/foo/ /usr/bin/firefox is owned by firefox 8.0-1 /usr/bin/firefox is owned by firefox 8.0-1 error: No package owns / error: failed to read file '/nonexistent': No such file or directory

From 280c6f32deed745a21ff3d059b7b956370425f6e Mon Sep 17 00:00:00 2001 From: Andrew Gregory <andrew.gregory.8@gmail.com> Date: Sun, 20 Nov 2011 12:01:15 -0500 Subject: [PATCH] allow querying fileowner for directories

Ahh, now I see- if you don't inline this but use it as the message...

Not allowing fileowner queries for directories was an unnecessary limitation. Because more than one package will likely own a given directory, all packages are checked when querying a directory instead of bailing out after the first owner is found.

Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---

 src/pacman/pacman.c |    2 +-  src/pacman/query.c  |   24 ++++++-------  src/pacman/util.c   |   97 +++++++++++++++++++++++++++++++++++---------------  src/pacman/util.h   |    4 +-  4 files changed, 82 insertions(+), 45 deletions(-)

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index fa35e8d..4b356fb 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -657,7 +657,7 @@ static int parseargs(int argc, char *argv[])                return 1;        }        if(config->help) { -               usage(config->op, mbasename(argv[0])); +               usage(config->op, pbasename(argv[0]));                return 2;        }        if(config->version) { diff --git a/src/pacman/query.c b/src/pacman/query.c index 4c2ea81..8bbce65 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; @@ -142,6 +142,7 @@ static int query_fileowner(alpm_list_t *targets)                struct stat buf;                alpm_list_t *i;                int found = 0; +               int searchall = 0;

               filename = strdup(t->data);

@@ -165,15 +166,11 @@ static int query_fileowner(alpm_list_t *targets)                }

               if(S_ISDIR(buf.st_mode)) { -                       pm_printf(ALPM_LOG_ERROR, -                               _("cannot determine ownership of directory '%s'\n"), filename); -                       ret++; -                       free(filename); -                       continue; +                       searchall = 1;                } searchall = S_ISDIR(buf.st_mode); would be a nice one-liner to replace

...all your extra comments can go here and they get discarded when the patch is applied. Right now I have no idea what is even going to show up if I tried to apply this patch from email directly. this whole block.

-               bname = mbasename(filename); -               dname = mdirname(filename); +               bname = pbasename(filename); +               dname = pdirname(filename);                /* for files in '/', there is no directory name to match */                if(strcmp(dname, "") == 0) {                        rpath = NULL; @@ -192,7 +189,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 && (searchall || !found); i = alpm_list_next(i)) {                        alpm_pkg_t *info = i->data;                        alpm_filelist_t *filelist = alpm_pkg_get_files(info);                        size_t j; @@ -203,7 +200,7 @@ static int query_fileowner(alpm_list_t *targets)                                const char *pkgfile = file->name;

                               /* avoid the costly resolve_path usage if the basenames don't match */ -                               if(strcmp(mbasename(pkgfile), bname) != 0) { +                               if(strcmp(pbasename(pkgfile), bname) != 0) {                                        continue;                                }

@@ -211,22 +208,23 @@ static int query_fileowner(alpm_list_t *targets)                                if(!rpath) {                                        print_query_fileowner(filename, info);                                        found = 1; -                                       continue; +                                       break;                                }

-                               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 */                                strcpy(path + rootlen, pkgfile);

-                               pdname = mdirname(path); +                               pdname = pdirname(path);                                ppath = resolve_path(pdname);                                free(pdname);

                               if(ppath && strcmp(ppath, rpath) == 0) {                                        print_query_fileowner(filename, info);                                        found = 1; +                                       break;                                }                                free(ppath);                        } diff --git a/src/pacman/util.c b/src/pacman/util.c index c0dcb9f..d5b9bd7 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -208,46 +208,85 @@ int rmrf(const char *path)        }  }

-/** Parse the basename of a program from a path. -* @param path path to parse basename from -* -* @return everything following the final '/' -*/ -const char *mbasename(const char *path) +/** Parse the basename from a path. + * Implements POSIX basename. + * Differences from POSIX:

These two lines immediately after each other don't make a whole lot of sense. "Implements POSIX basename with the following exceptions" or something would be better.

+ *   the basename returned should be freed + *   path is never modified + *   subsequent calls never modify previously returned results + * @param path path to parse parent from + * @return "." if path is NULL, basename of path otherwise + */ +char *pbasename(const char *path)  { -       const char *last = strrchr(path, '/'); -       if(last) { -               return last + 1; +       if(path == NULL || path == '\0') { +               return strdup("."); +       } + +       /* copy path so it doesn't get modified */ +       char *cpath = strdup(path); I know we do a bad job at it in src/, but in a util function, guarding against NULL would be nice in here and other strdup calls. +       char *last = cpath + (strlen(cpath) - 1); + +       /* move left of any trailing '/' */ +       while(*last == '/' && last != cpath){ Space between ) and {... +               --last; We never use prefix operators, last--; please. +       } + +       /* end string at last trailing '/' (or just overwrite '\0' if +        * there were no trailing '/') */ "there was" +       *(last+1) = '\0'; Please follow convention- always use spaces between multiple-arg operators. + +       /* find next '/' to the left */ +       while(*last != '/' && last != cpath){ +               --last; +       } + +       /* didn't find another '/', no parent dir */ +       if(*last != '/') { +               free(cpath); +               return strdup("."); This is a bit of a odd case, but to save yourself a free/alloc cycle and having to NULL check, you could just set cpath[0] to '.' and cpath[1] to '\0' (I think you can be sure your duped string is at least that long).        } -       return path; + +       return last+1;  }

Similar comments likely apply to the coding style below, so I didn't review it. If HACKING isn't clear about these things, I definitely encourage patches or even suggestions. It isn't fun for me to have to play style police when this is so easy to take care of when writing the patch, and I know it doesn't make you very excited to see a huge list of complaints in response to a patch.

-/** Parse the dirname of a program from a path. -* The path returned should be freed. -* @param path path to parse dirname from -* -* @return everything preceding the final '/' -*/ -char *mdirname(const char *path) +/** Parse the parent directory from a path. + * Implements POSIX dirname. + * Differences from POSIX: + *   the path returned should be freed + *   path is never modified + *   subsequent calls never modify previously returned results + * @param path path to parse parent from + * @return "." if path is NULL, parent directory of path otherwise + */ +char *pdirname(const char *path)  { -       char *ret, *last; - -       /* null or empty path */        if(path == NULL || path == '\0') {                return strdup(".");        }

-       ret = strdup(path); -       last = strrchr(ret, '/'); +       /* copy path so it doesn't get modified */ +       char *cpath = strdup(path); +       char *last = cpath + (strlen(cpath) - 1);

-       if(last != NULL) { -               /* we found a '/', so terminate our string */ -               *last = '\0'; -               return ret; +       /* move left of any trailing '/' */ +       while(*last == '/' && last != cpath){ +               --last;        } -       /* no slash found */ -       free(ret); -       return strdup("."); + +       /* find next '/' to the left */ +       while(*last != '/' && last != cpath){ +               --last; +       } + +       /* didn't find another '/', no parent dir */ +       if(*last != '/') { +               free(cpath); +               return strdup("."); +       } + +       *last = '\0'; +       return cpath;  }

 /* output a string, but wrap words properly with a specified indentation diff --git a/src/pacman/util.h b/src/pacman/util.h index 6ec962f..eaa3c40 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -52,8 +52,8 @@ int needs_root(void);  int check_syncdbs(size_t need_repos, int check_valid);  unsigned short getcols(void);  int rmrf(const char *path); -const char *mbasename(const char *path); -char *mdirname(const char *path); +char *pbasename(const char *path); +char *pdirname(const char *path);  void indentprint(const char *str, size_t indent);  char *strtoupper(char *str);  char *strtrim(char *str); -- 1.7.7.4

On Tue, Nov 22, 2011 at 8:12 PM, <andrew.gregory.8@gmail.com> wrote:

From: Andrew Gregory <andrew.gregory.8@gmail.com>

Removed restriction on querying the owner of a directory. Replaced mbasename and mdirname with pbasename and pdirname, similar to their POSIX namesakes except that they do not modify path, subsequent calls do not modify previous return values, and the returned values need to be freed. One potential gotcha is that if a symlink is queried using '.' or '..' it will first be resolved to its target directory. For Example: cd /var/mail pacman -Qo . /var/spool/mail is owned by filesystem ... pacman -Qo ../mail /var/mail is owned by filesystem...

Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---

Hope I got the patch formatting right this time. Quick test results below where you can see the 'gotcha' described in the commit.

Test Results: ln -s /usr/share /tmp/foo cd /tmp pacman -Qo /var/spool/mail /var/spool/mail/ /var/mail /var/mail/ /tmp/foo /tmp/foo/ firefox /usr/bin/firefox / /nonexistent /var/mail/. /var/mail/./ /tmp . .. ./..

/var/spool/mail is owned by filesystem 2011.10-1 /var/spool/mail/ is owned by filesystem 2011.10-1 /var/mail is owned by filesystem 2011.10-1 /var/mail/ is owned by filesystem 2011.10-1 error: No package owns /tmp/foo error: No package owns /tmp/foo/ /usr/bin/firefox is owned by firefox 8.0-1 /usr/bin/firefox is owned by firefox 8.0-1 error: No package owns / error: failed to read file '/nonexistent': No such file or directory /var/spool/mail is owned by filesystem 2011.10-1 /var/spool/mail is owned by filesystem 2011.10-1 /tmp is owned by filesystem 2011.10-1 /tmp is owned by filesystem 2011.10-1 error: No package owns / error: No package owns /

src/pacman/pacman.c | 2 +- src/pacman/query.c | 75 +++++++++++++++++++++---------------------- src/pacman/util.c | 89 ++++++++++++++++++++++++++++++++++---------------- src/pacman/util.h | 4 +- 4 files changed, 100 insertions(+), 70 deletions(-)

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index fa35e8d..4b356fb 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -657,7 +657,7 @@ static int parseargs(int argc, char *argv[]) return 1; } if(config->help) { - usage(config->op, mbasename(argv[0])); + usage(config->op, pbasename(argv[0])); return 2; } if(config->version) { diff --git a/src/pacman/query.c b/src/pacman/query.c index 4c2ea81..2769dc3 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; @@ -137,11 +137,11 @@ static int query_fileowner(alpm_list_t *targets) db_local = alpm_option_get_localdb(config->handle);

for(t = targets; t; t = alpm_list_next(t)) { - char *filename, *dname, *rpath; - const char *bname; + char *filename, *bname, *dname, *rpath = NULL; struct stat buf; alpm_list_t *i; int found = 0; + int searchall = 0;

filename = strdup(t->data);

@@ -164,69 +164,67 @@ static int query_fileowner(alpm_list_t *targets) } }

- if(S_ISDIR(buf.st_mode)) { - pm_printf(ALPM_LOG_ERROR, - _("cannot determine ownership of directory '%s'\n"), filename); - ret++; - free(filename); - continue; - } + searchall = S_ISDIR(buf.st_mode);

- bname = mbasename(filename); - dname = mdirname(filename); - /* for files in '/', there is no directory name to match */ - if(strcmp(dname, "") == 0) { - rpath = NULL; - } else { - rpath = resolve_path(dname); + bname = pbasename(filename); + /* if the basename is given as '.' or '..' we need to get the + * actual basename */ + if(strcmp(bname, ".") == 0 || strcmp(bname, "..") == 0) { + char *oldfilename = filename; + filename = resolve_path(oldfilename); + free(oldfilename);

- if(!rpath) { - pm_printf(ALPM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), - filename, strerror(errno)); - free(filename); - free(dname); - free(rpath); - ret++; - continue; - } + free(bname); + bname = pbasename(filename); } + + dname = pdirname(filename); + rpath = resolve_path(dname); free(dname);

- for(i = alpm_db_get_pkgcache(db_local); i && !found; i = alpm_list_next(i)) { + if(!rpath) { + pm_printf(ALPM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), + filename, strerror(errno)); + free(filename); + free(bname); + free(rpath); + ret++; + continue; + } + + for(i = alpm_db_get_pkgcache(db_local); i && (searchall || !found); i = alpm_list_next(i)) { alpm_pkg_t *info = i->data; alpm_filelist_t *filelist = alpm_pkg_get_files(info); size_t j;

for(j = 0; j < filelist->count; j++) { const alpm_file_t *file = filelist->files + j; - char *ppath, *pdname; + char *ppath, *pbname, *pdname; const char *pkgfile = file->name;

/* avoid the costly resolve_path usage if the basenames don't match */ - if(strcmp(mbasename(pkgfile), bname) != 0) { + pbname = pbasename(pkgfile); + if(strcmp(pbname, bname) != 0) { + free(pbname); continue; } + free(pbname);

- /* for files in '/', there is no directory name to match */ - if(!rpath) { - print_query_fileowner(filename, info); - found = 1; - 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 */ strcpy(path + rootlen, pkgfile);

- pdname = mdirname(path); + pdname = pdirname(path); ppath = resolve_path(pdname); free(pdname);

if(ppath && strcmp(ppath, rpath) == 0) { print_query_fileowner(filename, info); + free(ppath); found = 1; + break; } free(ppath); } @@ -236,6 +234,7 @@ static int query_fileowner(alpm_list_t *targets) ret++; } free(filename); + free(bname); free(rpath); }

diff --git a/src/pacman/util.c b/src/pacman/util.c index c0dcb9f..7f480ccb 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -208,46 +208,77 @@ int rmrf(const char *path) } }

-/** Parse the basename of a program from a path. -* @param path path to parse basename from -* -* @return everything following the final '/' -*/ -const char *mbasename(const char *path) +/** Parse the basename from a path. + * Implements POSIX basename with the following exceptions: + * the basename returned should be freed + * path is never modified + * subsequent calls never modify previously returned results + * @param path path to parse parent from + * @return NULL on error, "." if path is NULL, basename of path otherwise + */ +char *pbasename(const char *path) { - const char *last = strrchr(path, '/'); - if(last) { - return last + 1; + if(path == NULL || path == '\0') { + return strdup("."); + } + + const char *last = path + strlen(path) - 1; + + /* move left of any trailing '/' */ + while(*last == '/' && last != path) { + last--; + } + + /* if we made it all the way to the beginning whatever's there has to be our + * basename */ + if(last == path) { + return strndup(last, 1); + } + + const char *first = last; + /* find first '/' to the left */ + while(*first != '/' && first != path) { + first--; + } + if(*first == '/') { + first++; } - return path; + + return strndup(first, last - first + 1); }

-/** Parse the dirname of a program from a path. -* The path returned should be freed. -* @param path path to parse dirname from -* -* @return everything preceding the final '/' -*/ -char *mdirname(const char *path) +/** Parse the parent directory from a path. + * Implements POSIX dirname with the following exceptions: + * the path returned should be freed + * path is never modified + * subsequent calls never modify previously returned results + * @param path path to parse parent from + * @return NULL on error, "." if path is NULL, parent directory of path otherwise + */ +char *pdirname(const char *path) { - char *ret, *last; - - /* null or empty path */ if(path == NULL || path == '\0') { return strdup("."); }

- ret = strdup(path); - last = strrchr(ret, '/'); + const char *last = path + strlen(path) - 1;

- if(last != NULL) { - /* we found a '/', so terminate our string */ - *last = '\0'; - return ret; + /* move left of any trailing '/' */ + while(*last == '/' && last != path) { + last--; + } + + /* find next '/' to the left */ + while(*last != '/' && last != path) { + last--; } - /* no slash found */ - free(ret); - return strdup("."); + + /* didn't find another '/', no parent dir */ + if(*last != '/') { + return strdup("."); + } + + return strndup(path, last - path + 1); }

/* output a string, but wrap words properly with a specified indentation diff --git a/src/pacman/util.h b/src/pacman/util.h index 6ec962f..eaa3c40 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -52,8 +52,8 @@ int needs_root(void); int check_syncdbs(size_t need_repos, int check_valid); unsigned short getcols(void); int rmrf(const char *path); -const char *mbasename(const char *path); -char *mdirname(const char *path); +char *pbasename(const char *path); +char *pdirname(const char *path); void indentprint(const char *str, size_t indent); char *strtoupper(char *str); char *strtrim(char *str); -- 1.7.7.4

Haven't seen any movement on this yet. Were there any other concerns I did not address?