[pacman-dev] [PATCH] allow querying fileowner for directories
Dan McGee
dpmcgee at gmail.com
Tue Nov 22 12:20:38 EST 2011
On Tue, Nov 22, 2011 at 10:57 AM, Andrew Gregory
<andrew.gregory.8 at gmail.com> wrote:
> Think I've got it all worked out now. Replaced mdirname and mbasename
> with posix equivalent (but safer) functions. Directories no longer
> need to be treated any differently than files other than setting
> searchall. I'd like to suggest one more time that we go ahead and
> search all packages for files too even though more than one package
> *shouldn't* own a file. Otherwise, here's an updated patch.
Half of this is a commit message, half is commentary. Git provides an
easy way to address this- if you simply write your comments where I
indicate below, they won't be included when I git-am the patch. Can
you resubmit with a commit message that doesn't depend on context that
isn't there in the repository in a year, please?
(the test results are definitely fine; but the "updated patch",
"worked out now", etc. bits are all unnecessary)
>
> Test Results:
> ln -s /usr/share /tmp/foo
> /usr/local/bin/pacman -Qo /var/spool/mail /var/spool/mail/ /var/mail
> /var/mail/ /tmp/foo /tmp/foo/ firefox /usr/bin/firefox / /nonexistent
>
> /var/spool/mail is owned by filesystem 2011.10-1
> /var/spool/mail/ is owned by filesystem 2011.10-1
> /var/mail is owned by filesystem 2011.10-1
> /var/mail/ is owned by filesystem 2011.10-1
> error: No package owns /tmp/foo
> error: No package owns /tmp/foo/
> /usr/bin/firefox is owned by firefox 8.0-1
> /usr/bin/firefox is owned by firefox 8.0-1
> error: No package owns /
> error: failed to read file '/nonexistent': No such file or directory
>
> From 280c6f32deed745a21ff3d059b7b956370425f6e Mon Sep 17 00:00:00 2001
> From: Andrew Gregory <andrew.gregory.8 at gmail.com>
> Date: Sun, 20 Nov 2011 12:01:15 -0500
> Subject: [PATCH] allow querying fileowner for directories
Ahh, now I see- if you don't inline this but use it as the message...
>
> Not allowing fileowner queries for directories was an unnecessary
> limitation. Because more than one package will likely own a given
> directory, all packages are checked when querying a directory instead
> of bailing out after the first owner is found.
>
> Signed-off-by: Andrew Gregory <andrew.gregory.8 at gmail.com>
> ---
...all your extra comments can go here and they get discarded when the
patch is applied. Right now I have no idea what is even going to show
up if I tried to apply this patch from email directly.
> src/pacman/pacman.c | 2 +-
> src/pacman/query.c | 24 ++++++-------
> src/pacman/util.c | 97 +++++++++++++++++++++++++++++++++++---------------
> src/pacman/util.h | 4 +-
> 4 files changed, 82 insertions(+), 45 deletions(-)
>
> diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c
> index fa35e8d..4b356fb 100644
> --- a/src/pacman/pacman.c
> +++ b/src/pacman/pacman.c
> @@ -657,7 +657,7 @@ static int parseargs(int argc, char *argv[])
> return 1;
> }
> if(config->help) {
> - usage(config->op, mbasename(argv[0]));
> + usage(config->op, pbasename(argv[0]));
> return 2;
> }
> if(config->version) {
> diff --git a/src/pacman/query.c b/src/pacman/query.c
> index 4c2ea81..8bbce65 100644
> --- a/src/pacman/query.c
> +++ b/src/pacman/query.c
> @@ -127,7 +127,7 @@ static int query_fileowner(alpm_list_t *targets)
> * iteration. */
> root = alpm_option_get_root(config->handle);
> rootlen = strlen(root);
> - if(rootlen + 1 > PATH_MAX) {
> + if(rootlen >= PATH_MAX) {
> /* we are in trouble here */
> pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"), root, "");
> return 1;
> @@ -142,6 +142,7 @@ static int query_fileowner(alpm_list_t *targets)
> struct stat buf;
> alpm_list_t *i;
> int found = 0;
> + int searchall = 0;
>
> filename = strdup(t->data);
>
> @@ -165,15 +166,11 @@ static int query_fileowner(alpm_list_t *targets)
> }
>
> if(S_ISDIR(buf.st_mode)) {
> - pm_printf(ALPM_LOG_ERROR,
> - _("cannot determine ownership of directory '%s'\n"), filename);
> - ret++;
> - free(filename);
> - continue;
> + searchall = 1;
> }
searchall = S_ISDIR(buf.st_mode); would be a nice one-liner to replace
this whole block.
>
> - bname = mbasename(filename);
> - dname = mdirname(filename);
> + bname = pbasename(filename);
> + dname = pdirname(filename);
> /* for files in '/', there is no directory name to match */
> if(strcmp(dname, "") == 0) {
> rpath = NULL;
> @@ -192,7 +189,7 @@ static int query_fileowner(alpm_list_t *targets)
> }
> free(dname);
>
> - for(i = alpm_db_get_pkgcache(db_local); i && !found; i = alpm_list_next(i)) {
> + for(i = alpm_db_get_pkgcache(db_local); i && (searchall || !found); i = alpm_list_next(i)) {
> alpm_pkg_t *info = i->data;
> alpm_filelist_t *filelist = alpm_pkg_get_files(info);
> size_t j;
> @@ -203,7 +200,7 @@ static int query_fileowner(alpm_list_t *targets)
> const char *pkgfile = file->name;
>
> /* avoid the costly resolve_path usage if the basenames don't match */
> - if(strcmp(mbasename(pkgfile), bname) != 0) {
> + if(strcmp(pbasename(pkgfile), bname) != 0) {
> continue;
> }
>
> @@ -211,22 +208,23 @@ static int query_fileowner(alpm_list_t *targets)
> if(!rpath) {
> print_query_fileowner(filename, info);
> found = 1;
> - continue;
> + break;
> }
>
> - if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) {
> + if(rootlen + strlen(pkgfile) >= PATH_MAX) {
> pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"), root, pkgfile);
> }
> /* concatenate our file and the root path */
> strcpy(path + rootlen, pkgfile);
>
> - pdname = mdirname(path);
> + pdname = pdirname(path);
> ppath = resolve_path(pdname);
> free(pdname);
>
> if(ppath && strcmp(ppath, rpath) == 0) {
> print_query_fileowner(filename, info);
> found = 1;
> + break;
> }
> free(ppath);
> }
> diff --git a/src/pacman/util.c b/src/pacman/util.c
> index c0dcb9f..d5b9bd7 100644
> --- a/src/pacman/util.c
> +++ b/src/pacman/util.c
> @@ -208,46 +208,85 @@ int rmrf(const char *path)
> }
> }
>
> -/** Parse the basename of a program from a path.
> -* @param path path to parse basename from
> -*
> -* @return everything following the final '/'
> -*/
> -const char *mbasename(const char *path)
> +/** Parse the basename from a path.
> + * Implements POSIX basename.
> + * Differences from POSIX:
These two lines immediately after each other don't make a whole lot of sense.
"Implements POSIX basename with the following exceptions" or something
would be better.
> + * the basename returned should be freed
> + * path is never modified
> + * subsequent calls never modify previously returned results
> + * @param path path to parse parent from
> + * @return "." if path is NULL, basename of path otherwise
> + */
> +char *pbasename(const char *path)
> {
> - const char *last = strrchr(path, '/');
> - if(last) {
> - return last + 1;
> + if(path == NULL || path == '\0') {
> + return strdup(".");
> + }
> +
> + /* copy path so it doesn't get modified */
> + char *cpath = strdup(path);
I know we do a bad job at it in src/, but in a util function, guarding
against NULL would be nice in here and other strdup calls.
> + char *last = cpath + (strlen(cpath) - 1);
> +
> + /* move left of any trailing '/' */
> + while(*last == '/' && last != cpath){
Space between ) and {...
> + --last;
We never use prefix operators, last--; please.
> + }
> +
> + /* end string at last trailing '/' (or just overwrite '\0' if
> + * there were no trailing '/') */
"there was"
> + *(last+1) = '\0';
Please follow convention- always use spaces between multiple-arg operators.
> +
> + /* find next '/' to the left */
> + while(*last != '/' && last != cpath){
> + --last;
> + }
> +
> + /* didn't find another '/', no parent dir */
> + if(*last != '/') {
> + free(cpath);
> + return strdup(".");
This is a bit of a odd case, but to save yourself a free/alloc cycle
and having to NULL check, you could just set cpath[0] to '.' and
cpath[1] to '\0' (I think you can be sure your duped string is at
least that long).
> }
> - return path;
> +
> + return last+1;
> }
Similar comments likely apply to the coding style below, so I didn't review it.
If HACKING isn't clear about these things, I definitely encourage
patches or even suggestions. It isn't fun for me to have to play style
police when this is so easy to take care of when writing the patch,
and I know it doesn't make you very excited to see a huge list of
complaints in response to a patch.
> -/** Parse the dirname of a program from a path.
> -* The path returned should be freed.
> -* @param path path to parse dirname from
> -*
> -* @return everything preceding the final '/'
> -*/
> -char *mdirname(const char *path)
> +/** Parse the parent directory from a path.
> + * Implements POSIX dirname.
> + * Differences from POSIX:
> + * the path returned should be freed
> + * path is never modified
> + * subsequent calls never modify previously returned results
> + * @param path path to parse parent from
> + * @return "." if path is NULL, parent directory of path otherwise
> + */
> +char *pdirname(const char *path)
> {
> - char *ret, *last;
> -
> - /* null or empty path */
> if(path == NULL || path == '\0') {
> return strdup(".");
> }
>
> - ret = strdup(path);
> - last = strrchr(ret, '/');
> + /* copy path so it doesn't get modified */
> + char *cpath = strdup(path);
> + char *last = cpath + (strlen(cpath) - 1);
>
> - if(last != NULL) {
> - /* we found a '/', so terminate our string */
> - *last = '\0';
> - return ret;
> + /* move left of any trailing '/' */
> + while(*last == '/' && last != cpath){
> + --last;
> }
> - /* no slash found */
> - free(ret);
> - return strdup(".");
> +
> + /* find next '/' to the left */
> + while(*last != '/' && last != cpath){
> + --last;
> + }
> +
> + /* didn't find another '/', no parent dir */
> + if(*last != '/') {
> + free(cpath);
> + return strdup(".");
> + }
> +
> + *last = '\0';
> + return cpath;
> }
>
> /* output a string, but wrap words properly with a specified indentation
> diff --git a/src/pacman/util.h b/src/pacman/util.h
> index 6ec962f..eaa3c40 100644
> --- a/src/pacman/util.h
> +++ b/src/pacman/util.h
> @@ -52,8 +52,8 @@ int needs_root(void);
> int check_syncdbs(size_t need_repos, int check_valid);
> unsigned short getcols(void);
> int rmrf(const char *path);
> -const char *mbasename(const char *path);
> -char *mdirname(const char *path);
> +char *pbasename(const char *path);
> +char *pdirname(const char *path);
> void indentprint(const char *str, size_t indent);
> char *strtoupper(char *str);
> char *strtrim(char *str);
> --
> 1.7.7.4
More information about the pacman-dev
mailing list