[pacman-dev] [PATCH] Allow -Qo to perform a functional 'which'

[Dan McGee]

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.

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

> +
> +       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?

> +                       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?

> +
>        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?

-Dan