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

[Dan McGee]

On Fri, Nov 7, 2008 at 2:51 AM, Jatheendra <jatheendra at gmail.com> wrote:
> On Fri, Nov 7, 2008 at 1:18 PM, Jatheendra <jatheendra at gmail.com> wrote:
>> On Thu, Nov 6, 2008 at 10:17 PM, Allan McRae <allan at archlinux.org> wrote:
>>> Jatheendra wrote:
>>>>
>>>> Hi all,
>>>>
>>>> This is a simple patch to add a "which" like functionality to pacman
>>>> -Qo[Bug FS#8798]. This is my first patch ,so any constructive
>>>> criticism is most welcome.
>>>>
>>>> Thanks,
>>>> Shankar
>>>>
>>>
>>> Nice work.  This is a really nice feature to implement.
>>>
>>>
>>>> ---
>>>>  src/pacman/query.c |   62
>>>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>  1 files changed, 57 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/pacman/query.c b/src/pacman/query.c
>>>> index 0d48638..fb3bb92 100644
>>>> --- a/src/pacman/query.c
>>>> +++ b/src/pacman/query.c
>>>> @@ -55,6 +55,43 @@ 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, *path, *fullname;
>>>> +       int len;
>>>> +
>>>> +       if ((envpath = getenv("PATH")) == NULL) {
>>>> +               return(-1);
>>>> +       }
>>>> +
>>>> +       if ((envpath = strdup(envpath)) == NULL) {
>>>> +               return(-1);
>>>> +       }
>>>> +
>>>>
>>>
>>> Do we need the strdup here?  I realize that getenv can return static data
>>> the may be overwritten by the next getenv, but we are not calling getenv
>>> again.  Or how about an all in one strdup(getenv("PATH"))?
>>
>> The problem was that subsequent calls to getenv are returning the same
>> mem location. And we are mutilating that location (envpath) by calling
>> strsep.
>> So if we have more than one targets passed to Qo , then from second
>> target onwards it will search only along the mutilated path..
>> I dont know if it is some mistake i made or the implimentation of getenv.
>>
> >From man getenv
>  As typically implemented, getenv() returns a pointer to a string
> within the environment list.  The caller must take care not to modify
> this string, since that would change the environment of the process.
>
>> About strdup(getenv("PATH")) , will it be possible to handle the error
>> if getenv fails ? I am not sure what strdup(NULL) will do..
>>
>>
>>>
>>>> +       fullname = calloc(PATH_MAX+1, sizeof(char));
>>>> +
>>>> +       while ((path = strsep(&envpath, ":")) != NULL) {
>>>> +                       len = strlen(path);
>>>> +
>>>> +                       /* strip trailing '/' */
>>>> +                       while (path[len-1] == '/') {
>>>> +                                       path[--len] = '\0';
>>>> +                       }
>>>> +
>>>> +                       snprintf(fullname, PATH_MAX+1, "%s/%s", path,
>>>> filename);
>>>> +
>>>> +                       if(stat(fullname,bufptr) == 0) {
>>>> +                               strncpy(filename,fullname,PATH_MAX+1);
>>>> +                               free(fullname);
>>>> +                               return(0);
>>>> +                       }
>>>>
>>>
>>> +1 to Aaron's comment here.
>>>
>>>> +       }
>>>> +       free(fullname);
>>>> +       return(-1);
>>>> +}
>>>> +
>>>> +
>>>>  static int query_fileowner(alpm_list_t *targets)
>>>>  {
>>>>        int ret = 0;
>>>> @@ -66,21 +103,35 @@ static int query_fileowner(alpm_list_t *targets)
>>>>                return(1);
>>>>        }
>>>>
>>>> +       char *filename = calloc(PATH_MAX+1, sizeof(char));
>>>> +
>>>>        for(t = targets; t; t = alpm_list_next(t)) {
>>>>                int found = 0;
>>>> -               char *filename = alpm_list_getdata(t);
>>>> +               strncpy(filename,alpm_list_getdata(t),PATH_MAX+1);
>>>> +
>>>>                char *bname, *dname, *rpath;
>>>>                const char *root;
>>>>                struct stat buf;
>>>>                alpm_list_t *i, *j;
>>>>
>>>>                if(stat(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 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;
>>>> +                       }
>>>>
>>>
>>> Is there a way to have this error message not duplicated without making the
>>> if statement look really complicated?
>>
>> Will try :-)
>>
>>>
>>>>                }
>>>>
>>>> +
>>>>                if(S_ISDIR(buf.st_mode)) {
>>>>                        pm_fprintf(stderr, PM_LOG_ERROR,
>>>>                                _("cannot determine ownership of a
>>>> directory\n"));
>>>> @@ -140,6 +191,7 @@ static int query_fileowner(alpm_list_t *targets)
>>>>                free(rpath);
>>>>        }
>>>>
>>>> +       free(filename);
>>>>        return ret;
>>>>  }

This looks like it got dropped on the floor (but is linked in the
comments of FS#8798). Anyone want to dust it off and resubmit?

-Dan