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

[Jatheendra]

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.

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;
>>  }
>>
>>
>
>
>
> _______________________________________________
> pacman-dev mailing list
> pacman-dev at archlinux.org
> http://archlinux.org/mailman/listinfo/pacman-dev
>