FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux Pacman Development

 
 
LinkBack Thread Tools
 
Old 11-20-2011, 04:01 PM
Andrew Gregory
 
Default allow querying fileowner for directories

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
"), 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'
"), filename);
- ret++;
- free(filename);
- continue;
+ searchall = 1;
}

- 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
"), 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:
+ * 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 == '') {
+ return strdup(".");
+ }
+
+ /* copy path so it doesn't get modified */
+ char *cpath = strdup(path);
+ char *last = cpath + (strlen(cpath) - 1);
+
+ /* move left of any trailing '/' */
+ while(*last == '/' && last != cpath){
+ --last;
+ }
+
+ /* end string at last trailing '/' (or just overwrite '' if
+ * there were no trailing '/') */
+ *(last+1) = '';
+
+ /* find next '/' to the left */
+ while(*last != '/' && last != cpath){
+ --last;
+ }
+
+ /* didn't find another '/', no parent dir */
+ if(*last != '/') {
+ free(cpath);
+ return strdup(".");
}
- return path;
+
+ return last+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.
+ * 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 == '') {
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 = '';
- 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 = '';
+ 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
 
Old 11-20-2011, 08:03 PM
 
Default allow querying fileowner for directories

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(-)

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
"), 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'
"), filename);
- ret++;
- free(filename);
- continue;
+ searchall = 1;
+ size_t fnlen = strlen(filename);
+ if(filename[fnlen-1] != '/'){
+ filename = realloc(filename, sizeof(char) * (fnlen+2));
+ strcat(filename, "/");
+ }
}

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)) {
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)
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
"), root, pkgfile);
}
/* concatenate our file and the root path */
--
1.7.7.3
 
Old 11-22-2011, 06:14 AM
Dan McGee
 
Default allow querying fileowner for directories

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
"), 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'
"), 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] = '' 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
"), root, pkgfile);
> * * * * * * * * * * * * * * * *}
> * * * * * * * * * * * * * * * */* concatenate our file and the root path */
> --
> 1.7.7.3
 
Old 11-22-2011, 03:57 PM
Andrew Gregory
 
Default allow querying fileowner for directories

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.

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
 
Old 11-22-2011, 04:20 PM
Dan McGee
 
Default allow querying fileowner for directories

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>
> ---
...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.

> *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
"), 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'
"), filename);
> - * * * * * * * * * * * ret++;
> - * * * * * * * * * * * free(filename);
> - * * * * * * * * * * * continue;
> + * * * * * * * * * * * searchall = 1;
> * * * * * * * *}
searchall = S_ISDIR(buf.st_mode); would be a nice one-liner to replace
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
"), 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 == '') {
> + * * * * * * * 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 '' if
> + * * * ** there were no trailing '/') */
"there was"
> + * * * *(last+1) = '';
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 '' (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 == '') {
> * * * * * * * *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 = '';
> - * * * * * * * 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 = '';
> + * * * 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
 
Old 11-23-2011, 12:12 AM
 
Default allow querying fileowner for directories

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
"), 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'
"), 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
"),
- 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
"),
+ 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
"), 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 == '') {
+ 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 == '') {
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 = '';
- 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
 
Old 12-23-2011, 03:29 AM
Andrew Gregory
 
Default allow querying fileowner for directories

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
"), 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'
"), 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
"),
> - 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
"),
> + 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
"), 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 == '') {
> + 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 == '') {
> 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 = '';
> - 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?
 

Thread Tools




All times are GMT. The time now is 05:55 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org