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
--- 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); + } + + 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); + } + } + 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; + } }
+ 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; }
On Wed, Nov 5, 2008 at 10:13 AM, Jatheendra jatheendra@gmail.com 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.
Oooh. Exciting!
The only comment I have might be wrong: I could have sworn we had a "path join" type function somewhere that dealt with slash and all that stuff. If we do, maybe we could use that... if not, maybe we should add one and use it for all our patch stuff that does the identical stuff (if trailing slash, remove, etc)
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"))?
- 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?
}
- 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;
}
On Thu, Nov 6, 2008 at 10:17 PM, Allan McRae allan@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 readfile '%s': %s\n"),
filename, strerror(errno));ret++;continue;
/* if it is not a path but a program name, thencheck 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, _("failedto 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 adirectory\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@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev
On Fri, Nov 7, 2008 at 1:18 PM, Jatheendra jatheendra@gmail.com wrote:
On Thu, Nov 6, 2008 at 10:17 PM, Allan McRae allan@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 readfile '%s': %s\n"),
filename, strerror(errno));ret++;continue;
/* if it is not a path but a program name, thencheck 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, _("failedto 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 adirectory\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@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev
On Fri, Nov 7, 2008 at 2:51 AM, Jatheendra jatheendra@gmail.com wrote:
On Fri, Nov 7, 2008 at 1:18 PM, Jatheendra jatheendra@gmail.com wrote:
On Thu, Nov 6, 2008 at 10:17 PM, Allan McRae allan@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
pacman-dev@lists.archlinux.org