On 06/05/10 11:03, Dan McGee wrote:

On Sat, Mar 6, 2010 at 10:02 PM, Allan McRae wrote:

...

When pacman queries the ownership of an object that is not a path, it will check in the users PATH for a match. Implements FS#8798.

Original-patch-by: Shankar Signed-off-by: Allan McRae ---

This should address all comments I had when this was submitted a few months back.

Until I'm about to merge it tonight! I have a few small ideas/tweaks. :P

...

src/pacman/query.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/src/pacman/query.c b/src/pacman/query.c index 6b6a25d..038072f 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -56,10 +56,44 @@ static char *resolve_path(const char* file) return(str); }

+/* check if filename exists in PATH */ +static int search_path(char *filename, size_t size, struct stat * bufptr) no space needed between * and bufptr +{ + char *envpath, *envpathsplit, *path, fullname[PATH_MAX+1];

Perhaps I am being a bit overkill, but I'd prefer: char *envpath, *envpathsplit, *path; char fullname[PATH_MAX+1];

just to separate the ptrs from the real thing.

Sure, but that is kindof the whole point of using "char *foo" rather than "char* foo"... I do not care so will separate.

...

+ int len; + + if ((envpath = getenv("PATH")) == NULL) { + return(-1); + }

Not that you'd see this regularly, but sudo can sometimes play weird games with envvars among other things. This will cause a rather nonsensical error:

$ PATH= ./src/pacman/.libs/lt-pacman -Qo pacman error: failed to read file 'pacman': No such file or directory

$ ./src/pacman/.libs/lt-pacman -Qo pacman /usr/bin/pacman is owned by pacman-git 20100404-1

...

+ if ((envpath = envpathsplit = strdup(envpath)) == NULL) { + return(-1); + }

Same kind of deal. I think we need better wording to indicate we attempted to search the $PATH and failed, as opposed to couldn't locate a file in the current directory and/or looked for an absolute file path.

$ ./src/pacman/.libs/lt-pacman -Qo foobar error: failed to read file 'foobar': No such file or directory

Agreed, the error message could be improved.

...

+ + while ((path = strsep(&envpathsplit, ":")) != NULL) { + len = strlen(path); + + /* strip the trailing slash if one exists */ + while(path[len - 1] == '/') { + path[--len] = '\0'; + } + + snprintf(fullname, sizeof(fullname), "%s/%s", path, filename); + + if(stat(fullname, bufptr) == 0) { Shouldn't we be using lstat just like the query_fileowner() code? Otherwise broken symlink from package X will be unqueryable?

Yes.

...

+ strncpy(filename, fullname, size); + free(envpath); + return(0); + } + } + free(envpath); + return(-1); +} + static int query_fileowner(alpm_list_t *targets) { int ret = 0; alpm_list_t *t; + size_t len=PATH_MAX+1; We use spaces between operators everywhere else; should use them here too please.

...

/* This code is here for safety only */ if(targets == NULL) { @@ -67,19 +101,31 @@ static int query_fileowner(alpm_list_t *targets) return(1); }

+ char *filename = calloc(len, sizeof(char));

Why do we go through the "trouble" of allocating this one on the heap when every single call to search_path() just allocates something the same size on the stack?

No idea... when trying to figure out the most "pacman" way to do all the memory management of character arrays, I spent a lot of time looking at other areas of the code. It seems there is a real mix of methods being used and I was never sure of the correct way. Would you prefer both on the stack or both on the heap?

...

+ for(t = targets; t; t = alpm_list_next(t)) { int found = 0; - char *filename = alpm_list_getdata(t); + strncpy(filename, alpm_list_getdata(t), len); char *bname, *dname, *rpath; const char *root; struct stat buf; alpm_list_t *i, *j;

if(lstat(filename,&buf) == -1) { - pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), - filename, strerror(errno)); - ret++; - continue; + /* if it is not a path but a program name, then check in PATH */ + if(strchr(filename, '/') == NULL) { + if(search_path(filename, len,&buf) == -1) { + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), + filename, strerror(errno)); + ret++; + continue; + } + } else { + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), + filename, strerror(errno)); + ret++; + continue; + } }

if(S_ISDIR(buf.st_mode)) { @@ -140,6 +186,7 @@ static int query_fileowner(alpm_list_t *targets) free(rpath); }

+ free(filename); return ret; }

Oh, and more weirdness (my working directory was ~/projects/pacman/ with no etc in sight):

$ ./src/pacman/.libs/lt-pacman -Qo etc error: cannot determine ownership of a directory

$ pacman -Qo etc error: cannot determine ownership of a directory

So this is not new, but worth looking at perhaps?

Ah... figured this out! It turns out that there is an etc directory in the folder you were in. Really! Take another look. :) This is the behaviour with pacman 3.3.x too and should probably be fixed (it should not look at a local directory). That is a separate issue so should probably be a separate patch. I will not be able to look at this until early next week (heading of camping tomorrow), so feel free to either wait or make the small adjustments needed. Allan

On 11/05/10 04:02, Dan McGee wrote:

From: Allan McRae

When pacman queries the ownership of an object that is not a path, it will check in the users PATH for a match. Implements FS#8798.

Dan: did some small refactoring and error message changes when PATH is searched and nothing is found.

Original-patch-by: Shankar Signed-off-by: Allan McRae Signed-off-by: Dan McGee ---

I think I just had some NIH and did a bit more refactoring than strictly necessary, but I thought I'd at least send this along. I think it gets all the error messaging tweaked up to be a bit more useful.

-Dan

Looks good to me. Couple of minor comments.

src/pacman/query.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/src/pacman/query.c b/src/pacman/query.c index 6010fd0..a882328 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -56,9 +56,49 @@ static char *resolve_path(const char* file) return(str); }

+/* check if filename exists in PATH */ +static int search_path(char **filename, struct stat *bufptr) +{ + char *envpath, *envpathsplit, *path; + char *fullname;

These are the same type now so can go back on one line.

+ size_t flen; + + if ((envpath = getenv("PATH")) == NULL) { + return(-1); + } + if ((envpath = envpathsplit = strdup(envpath)) == NULL) { + return(-1); + } + + flen = strlen(*filename); + + while ((path = strsep(&envpathsplit, ":")) != NULL) { + size_t plen = strlen(path); + + /* strip the trailing slash if one exists */ + while(path[plen - 1] == '/') { + path[--plen] = '\0'; + } + + fullname = malloc(plen + flen + 2); + sprintf(fullname, "%s/%s", path, *filename); + + if(lstat(fullname, bufptr) == 0) { + free(*filename); + *filename = fullname; + free(envpath); + return(0); + } + free(fullname); + } + free(envpath); + return(-1); +} + static int query_fileowner(alpm_list_t *targets) { int ret = 0; + char *filename; alpm_list_t *t;

/* This code is here for safety only */ @@ -69,23 +109,36 @@ static int query_fileowner(alpm_list_t *targets)

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

if(lstat(filename,&buf) == -1) { - pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), - filename, strerror(errno)); - ret++; - continue; + /* if it is not a path but a program name, then check in PATH */ + if(strchr(filename, '/') == NULL) { + if(search_path(&filename,&buf) == -1) { + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to find '%s' on PATH: %s\n"),

"on PATH" vs "in PATH"?

+ filename, strerror(errno)); + ret++; + free(filename); + continue; + } + } else { + pm_fprintf(stderr, PM_LOG_ERROR, _("failed to read file '%s': %s\n"), + filename, strerror(errno)); + ret++; + free(filename); + continue; + } }

if(S_ISDIR(buf.st_mode)) { pm_fprintf(stderr, PM_LOG_ERROR, _("cannot determine ownership of a directory\n")); ret++; + free(filename); continue; }

@@ -97,6 +150,7 @@ static int query_fileowner(alpm_list_t *targets) if(!rpath) { pm_fprintf(stderr, PM_LOG_ERROR, _("cannot determine real path for '%s': %s\n"), filename, strerror(errno)); + free(filename); free(rpath); ret++; continue; @@ -137,6 +191,7 @@ static int query_fileowner(alpm_list_t *targets) pm_fprintf(stderr, PM_LOG_ERROR, _("No package owns %s\n"), filename); ret++; } + free(filename); free(rpath); }