[pacman-dev] [PATCH 0/4] Miscellaneous query_fileowner cleanup

older
[pacman-dev] [RFC] Generation of a...

Andrew Gregory

11 Aug 2012 11 Aug '12

9:35 p.m.

These patches apply on top of my previous query.c patches sitting in Allan's working-split/query-owner branch. Also available under the dirowner_split branch of my repo at https://bitbucket.org/andrewgregory/pacman

Show replies by date

Andrew Gregory

11 Aug 11 Aug

9:35 p.m.

New subject: [pacman-dev] [PATCH 1/4] query_fileowner: remove assumption that root is "/"

Returning "/" from mdirname removes it as a special case which allows us to test it like any other directory. This corrects a false positive when querying a file in / and root is not set to /. Signed-off-by: Andrew Gregory --- src/pacman/query.c | 31 ++++++------------------------- src/pacman/util.c | 5 +++++ 2 files changed, 11 insertions(+), 25 deletions(-) diff --git a/src/pacman/query.c b/src/pacman/query.c index 11876d8..b1e51a6 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -173,21 +173,12 @@ static int query_fileowner(alpm_list_t *targets) bname = mbasename(filename); dname = mdirname(filename); - /* for files in '/', there is no directory name to match */ - if(strcmp(dname, "") == 0) { - rpath = NULL; - } else { - rpath = realpath(dname, NULL); + rpath = realpath(dname, NULL); - 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; - } + if(!dname || !rpath) { + pm_printf(ALPM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), + filename, strerror(errno)); + goto targcleanup; } free(dname); @@ -206,21 +197,11 @@ static int query_fileowner(alpm_list_t *targets) continue; } - /* for files in '/', there is no directory name to match */ - if(!rpath) { - if(strcmp(pkgfile, bname) == 0) { - print_query_fileowner(filename, info); - found = 1; - break; - } - continue; - } - + /* concatenate our file and the root path */ if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) { pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"), path, pkgfile); continue; } - /* concatenate our file and the root path */ strcpy(path + rootlen, pkgfile); pdname = mdirname(path); diff --git a/src/pacman/util.c b/src/pacman/util.c index 7f7f6a7..2d1b762 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -244,9 +244,14 @@ char *mdirname(const char *path) if(last != NULL) { /* we found a '/', so terminate our string */ + if(last == ret) { + /* return "/" for root */ + last++; + } *last = '\0'; return ret; } + /* no slash found */ free(ret); return strdup("."); -- 1.7.11.4

Allan McRae

27 Aug 27 Aug

7:24 a.m.

New subject: [pacman-dev] [PATCH 1/4] query_fileowner: remove assumption that root is "/"

On 12/08/12 07:35, Andrew Gregory wrote:

...

Returning "/" from mdirname removes it as a special case which allows us to test it like any other directory. This corrects a false positive when querying a file in / and root is not set to /.

Signed-off-by: Andrew Gregory --- src/pacman/query.c | 31 ++++++------------------------- src/pacman/util.c | 5 +++++ 2 files changed, 11 insertions(+), 25 deletions(-)

Ack-by: Allan Sorry it took so long to get to looking at these patches.

Andrew Gregory

11 Aug 11 Aug

9:35 p.m.

New subject: [pacman-dev] [PATCH 2/4] query_fileowner, mdirname: add error checks

Also consolidates cleanup for query_fileowner. Signed-off-by: Andrew Gregory --- src/pacman/query.c | 28 ++++++++++++++-------------- src/pacman/util.c | 5 ++++- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/pacman/query.c b/src/pacman/query.c index b1e51a6..9afe680 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -129,14 +129,16 @@ static int query_fileowner(alpm_list_t *targets) db_local = alpm_get_localdb(config->handle); for(t = targets; t; t = alpm_list_next(t)) { - char *filename, *dname, *rpath; + char *filename = NULL, *dname = NULL, *rpath = NULL; const char *bname; struct stat buf; alpm_list_t *i; size_t len; int found = 0; - filename = strdup(t->data); + if((filename = strdup(t->data)) == NULL) { + goto targcleanup; + } /* trailing '/' causes lstat to dereference directory symlinks */ len = strlen(filename) - 1; @@ -150,25 +152,19 @@ static int query_fileowner(alpm_list_t *targets) if(search_path(&filename, &buf) == -1) { pm_printf(ALPM_LOG_ERROR, _("failed to find '%s' in PATH: %s\n"), filename, strerror(errno)); - ret++; - free(filename); - continue; + goto targcleanup; } } else { pm_printf(ALPM_LOG_ERROR, _("failed to read file '%s': %s\n"), filename, strerror(errno)); - ret++; - free(filename); - continue; + goto targcleanup; } } if(S_ISDIR(buf.st_mode)) { pm_printf(ALPM_LOG_ERROR, _("cannot determine ownership of directory '%s'\n"), filename); - ret++; - free(filename); - continue; + goto targcleanup; } bname = mbasename(filename); @@ -180,7 +176,6 @@ static int query_fileowner(alpm_list_t *targets) filename, strerror(errno)); goto targcleanup; } - free(dname); for(i = alpm_db_get_pkgcache(db_local); i && !found; i = alpm_list_next(i)) { alpm_pkg_t *info = i->data; @@ -199,6 +194,7 @@ static int query_fileowner(alpm_list_t *targets) /* concatenate our file and the root path */ if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) { + path[rootlen] = '\0'; /* reset path for error message */ pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"), path, pkgfile); continue; } @@ -207,11 +203,10 @@ static int query_fileowner(alpm_list_t *targets) pdname = mdirname(path); ppath = realpath(pdname, NULL); free(pdname); - path[rootlen] = '\0'; /* reset path for error messages */ if(!ppath) { pm_printf(ALPM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), - pdname, strerror(errno)); + path, strerror(errno)); continue; } @@ -226,10 +221,15 @@ static int query_fileowner(alpm_list_t *targets) } if(!found) { pm_printf(ALPM_LOG_ERROR, _("No package owns %s\n"), filename); + } + +targcleanup: + if(!found) { ret++; } free(filename); free(rpath); + free(dname); } return ret; diff --git a/src/pacman/util.c b/src/pacman/util.c index 2d1b762..ba7a8c0 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -239,7 +239,10 @@ char *mdirname(const char *path) return strdup("."); } - ret = strdup(path); + if((ret = strdup(path)) == NULL) { + return NULL; + } + last = strrchr(ret, '/'); if(last != NULL) { -- 1.7.11.4

Allan McRae

27 Aug 27 Aug

7:24 a.m.

New subject: [pacman-dev] [PATCH 2/4] query_fileowner, mdirname: add error checks

On 12/08/12 07:35, Andrew Gregory wrote:

...

Also consolidates cleanup for query_fileowner.

Signed-off-by: Andrew Gregory --- src/pacman/query.c | 28 ++++++++++++++-------------- src/pacman/util.c | 5 ++++- 2 files changed, 18 insertions(+), 15 deletions(-)

<snip>

...

diff --git a/src/pacman/util.c b/src/pacman/util.c index 2d1b762..ba7a8c0 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -239,7 +239,10 @@ char *mdirname(const char *path) return strdup("."); }

- ret = strdup(path); + if((ret = strdup(path)) == NULL) { + return NULL;

Whitespace error. Fixed on my working branch.

...

+ } + last = strrchr(ret, '/');

if(last != NULL) {

Andrew Gregory

11 Aug 11 Aug

9:35 p.m.

New subject: [pacman-dev] [PATCH 3/4] query_fileowner: don't append '/' if path is "/"

Signed-off-by: Andrew Gregory --- src/pacman/query.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/pacman/query.c b/src/pacman/query.c index 9afe680..d882e5a 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -123,8 +123,10 @@ static int query_fileowner(alpm_list_t *targets) } /* append trailing '/' removed by realpath */ - path[rootlen++] = '/'; - path[rootlen] = '\0'; + if(strcmp(path, "/") != 0) { + path[rootlen++] = '/'; + path[rootlen] = '\0'; + } db_local = alpm_get_localdb(config->handle); -- 1.7.11.4

Allan McRae

27 Aug 27 Aug

7:24 a.m.

New subject: [pacman-dev] [PATCH 3/4] query_fileowner: don't append '/' if path is "/"

On 12/08/12 07:35, Andrew Gregory wrote:

...

Signed-off-by: Andrew Gregory --- src/pacman/query.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/pacman/query.c b/src/pacman/query.c index 9afe680..d882e5a 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -123,8 +123,10 @@ static int query_fileowner(alpm_list_t *targets) }

/* append trailing '/' removed by realpath */ - path[rootlen++] = '/'; - path[rootlen] = '\0'; + if(strcmp(path, "/") != 0) {

Given this is the uncommon case, can we change this to the less expensive: if(rootlen == 1 && path[0] == '/')

...

+ path[rootlen++] = '/'; + path[rootlen] = '\0'; + }

db_local = alpm_get_localdb(config->handle);

Andrew Gregory

28 Aug 28 Aug

4:34 a.m.

New subject: [pacman-dev] [PATCH 3/4] query_fileowner: don't append '/' if path is "/"

On Mon, 27 Aug 2012 17:24:54 +1000 Allan McRae wrote:

...

On 12/08/12 07:35, Andrew Gregory wrote:

...
Signed-off-by: Andrew Gregory --- src/pacman/query.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/pacman/query.c b/src/pacman/query.c index 9afe680..d882e5a 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -123,8 +123,10 @@ static int query_fileowner(alpm_list_t *targets) }

/* append trailing '/' removed by realpath */ - path[rootlen++] = '/'; - path[rootlen] = '\0'; + if(strcmp(path, "/") != 0) {

Given this is the uncommon case, can we change this to the less expensive:

if(rootlen == 1 && path[0] == '/')

Sure, but we may as well make it more generalized too by just testing for a trailing '/' with: if(path[rootlen - 1] != '/')

...

...
+ path[rootlen++] = '/'; + path[rootlen] = '\0'; + }

db_local = alpm_get_localdb(config->handle);

Allan McRae

8:02 a.m.

New subject: [pacman-dev] [PATCH 3/4] query_fileowner: don't append '/' if path is "/"

On 28/08/12 14:34, Andrew Gregory wrote:

...

On Mon, 27 Aug 2012 17:24:54 +1000 Allan McRae wrote:

...
On 12/08/12 07:35, Andrew Gregory wrote:

...
Signed-off-by: Andrew Gregory --- src/pacman/query.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/pacman/query.c b/src/pacman/query.c index 9afe680..d882e5a 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -123,8 +123,10 @@ static int query_fileowner(alpm_list_t *targets) }

/* append trailing '/' removed by realpath */ - path[rootlen++] = '/'; - path[rootlen] = '\0'; + if(strcmp(path, "/") != 0) {

Given this is the uncommon case, can we change this to the less expensive:

if(rootlen == 1 && path[0] == '/')

Sure, but we may as well make it more generalized too by just testing for a trailing '/' with:

if(path[rootlen - 1] != '/')

Of course! Send it through. Allan

Andrew Gregory

29 Aug 29 Aug

1:59 a.m.

New subject: [pacman-dev] [PATCH] query_fileowner: don't append '/' if path is "/"

Signed-off-by: Andrew Gregory --- src/pacman/query.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/pacman/query.c b/src/pacman/query.c index fc2c90c..92219e8 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -123,8 +123,10 @@ static int query_fileowner(alpm_list_t *targets) } /* append trailing '/' removed by realpath */ - path[rootlen++] = '/'; - path[rootlen] = '\0'; + if(path[rootlen - 1] != '/') { + path[rootlen++] = '/'; + path[rootlen] = '\0'; + } db_local = alpm_get_localdb(config->handle); -- 1.7.12

Andrew Gregory

11 Aug 11 Aug

9:35 p.m.

New subject: [pacman-dev] [PATCH 4/4] query.c: fix coding standard violations

--- src/pacman/query.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pacman/query.c b/src/pacman/query.c index d882e5a..7381c4e 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -58,7 +58,7 @@ static int search_path(char **filename, struct stat *bufptr) /* strip the trailing slash if one exists */ while(path[plen - 1] == '/') { - path[--plen] = '\0'; + path[--plen] = '\0'; } fullname = malloc(plen + flen + 2); @@ -144,7 +144,7 @@ static int query_fileowner(alpm_list_t *targets) /* trailing '/' causes lstat to dereference directory symlinks */ len = strlen(filename) - 1; - while(len > 0 && filename[len] == '/'){ + while(len > 0 && filename[len] == '/') { filename[len--] = '\0'; } -- 1.7.11.4

4251

Age (days ago)

4269

Last active (days ago)

List overview

Download

10 comments

2 participants

participants (2)

Allan McRae
Andrew Gregory

[pacman-dev] [PATCH 0/4] Miscellaneous query_fileowner cleanup

tags

participants (2)