On 06/05/10 11:03, Dan McGee wrote:
On Sat, Mar 6, 2010 at 10:02 PM, Allan McRae<[email protected]> 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<[email protected]>
Signed-off-by: Allan McRae<[email protected]>
---
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
