[pacman-dev] [PATCH] Allow -Qo to perform a functional 'which'
Allan McRae
allan at archlinux.org
Thu May 6 03:42:31 CEST 2010
On 06/05/10 11:03, Dan McGee wrote:
> On Sat, Mar 6, 2010 at 10:02 PM, Allan McRae<allan at archlinux.org> 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<jatheendra at gmail.com>
>> Signed-off-by: Allan McRae<allan at archlinux.org>
>> ---
>>
>> 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
More information about the pacman-dev
mailing list