[pacman-dev] [PATCH] Allow querying directory ownership
The restriction of not checking the ownership of a directory is unnecessary given that all the package filelists contain this information. Remove this restriction, with the expectation that you might get multiple packages returned for a given directory. Additionally attempt to minimise the number of files getting through to the slow realpath call. This combines ideas from two patches that have been around for a long time. Original-work-by: Andrew Gregory <andrew.gregory.8@gmail.com> Original-work-by: Dan McGee <dan@archlinux.org> Signed-off-by: Allan McRae <allan@archlinux.org> --- src/pacman/query.c | 58 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/src/pacman/query.c b/src/pacman/query.c index 07509fa..cf14fab 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -113,6 +113,7 @@ static int query_fileowner(alpm_list_t *targets) size_t rootlen; alpm_list_t *t; alpm_db_t *db_local; + alpm_list_t *packages; /* This code is here for safety only */ if(targets == NULL) { @@ -133,13 +134,14 @@ static int query_fileowner(alpm_list_t *targets) strcpy(path, root); db_local = alpm_get_localdb(config->handle); + packages = alpm_db_get_pkgcache(db_local); for(t = targets; t; t = alpm_list_next(t)) { char *filename, *dname, *rpath; - const char *bname; + const char *bname, *rpath_bname = NULL; struct stat buf; alpm_list_t *i; - int found = 0; + size_t found = 0, isdir, bname_len, rpath_len, rpath_bname_len = 0; filename = strdup(t->data); @@ -162,16 +164,19 @@ 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; + /* make sure directories have a trailing '/' */ + if((isdir = S_ISDIR(buf.st_mode))) { + size_t len = strlen(filename); + if(filename[len-1] != '/') { + filename = realloc(filename, sizeof(char) * (len + 2)); + strcat(filename, "/"); + } } bname = mbasename(filename); + bname_len = strlen(bname); dname = mdirname(filename); + /* for files in '/', there is no directory name to match */ if(strcmp(dname, "") == 0) { rpath = NULL; @@ -189,8 +194,14 @@ static int query_fileowner(alpm_list_t *targets) } } free(dname); + rpath_len = strlen(rpath); - for(i = alpm_db_get_pkgcache(db_local); i && !found; i = alpm_list_next(i)) { + if(isdir) { + rpath_bname = mbasename(rpath); + rpath_bname_len = strlen(rpath_bname); + } + + for(i = packages; i && (!found || isdir); i = alpm_list_next(i)) { alpm_pkg_t *info = i->data; alpm_filelist_t *filelist = alpm_pkg_get_files(info); size_t j; @@ -199,17 +210,40 @@ static int query_fileowner(alpm_list_t *targets) const alpm_file_t *file = filelist->files + j; char *ppath, *pdname; const char *pkgfile = file->name; + size_t pkgfile_len = strlen(pkgfile); - /* avoid the costly resolve_path usage if the basenames don't match */ - if(strcmp(mbasename(pkgfile), bname) != 0) { + /* avoid the costly resolve_path usage if the basenames don't match; + * we can also cheat by comparing the final characters first and avoid + * a full string comparison */ + if(!isdir && (pkgfile[pkgfile_len - 1] != bname[bname_len - 1] || + strcmp(mbasename(pkgfile), bname) != 0)) { continue; } + if(isdir) { + /* check the filelist entry is a directory using its + * trailing '/' and compare final characters of directory + * names. */ + if(pkgfile[pkgfile_len - 1] != '/' || + pkgfile[pkgfile_len - 2] != rpath[rpath_len - 1]) { + continue; + } + + /* compare final directory portion */ + if(pkgfile_len - 1 < rpath_bname_len || + (pkgfile_len - 2 >= rpath_bname_len && + pkgfile[pkgfile_len - 2 - rpath_bname_len] != '/') || + strncmp(pkgfile + pkgfile_len - 1 - rpath_bname_len, + rpath_bname, rpath_bname_len)) { + continue; + } + } + /* for files in '/', there is no directory name to match */ if(!rpath) { print_query_fileowner(filename, info); found = 1; - continue; + break; } if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) { -- 1.7.10.2
On Tue, 22 May 2012 08:28:11 +1000 Allan McRae <allan@archlinux.org> wrote:
The restriction of not checking the ownership of a directory is unnecessary given that all the package filelists contain this information. Remove this restriction, with the expectation that you might get multiple packages returned for a given directory. Additionally attempt to minimise the number of files getting through to the slow realpath call.
This combines ideas from two patches that have been around for a long time.
Original-work-by: Andrew Gregory <andrew.gregory.8@gmail.com> Original-work-by: Dan McGee <dan@archlinux.org> Signed-off-by: Allan McRae <allan@archlinux.org>
I actually decided to try tackling this again recently too. If you're interested, you can see the most recent incarnation here: http://bitbucket.org/andrewgregory/pacman/changesets/tip/directory_owner It gets rid of a lot of the basename/dirname stuff which I think makes it a lot easier to follow.
On 22/05/12 11:20, Andrew Gregory wrote:
On Tue, 22 May 2012 08:28:11 +1000 Allan McRae <allan@archlinux.org> wrote:
The restriction of not checking the ownership of a directory is unnecessary given that all the package filelists contain this information. Remove this restriction, with the expectation that you might get multiple packages returned for a given directory. Additionally attempt to minimise the number of files getting through to the slow realpath call.
This combines ideas from two patches that have been around for a long time.
Original-work-by: Andrew Gregory <andrew.gregory.8@gmail.com> Original-work-by: Dan McGee <dan@archlinux.org> Signed-off-by: Allan McRae <allan@archlinux.org>
I actually decided to try tackling this again recently too. If you're interested, you can see the most recent incarnation here: http://bitbucket.org/andrewgregory/pacman/changesets/tip/directory_owner It gets rid of a lot of the basename/dirname stuff which I think makes it a lot easier to follow.
I had a quick look through your patch. It is quite a change to the code so there is not anything obvious for me to grab into this one. How far would your patch be from being ready? If the answer is really soon, and Dan likes the look of your changes, then I am quite happy for "my" patch to be ignored. Allan
On Tue, 22 May 2012 16:38:42 +1000 Allan McRae <allan@archlinux.org> wrote:
On 22/05/12 11:20, Andrew Gregory wrote:
On Tue, 22 May 2012 08:28:11 +1000 Allan McRae <allan@archlinux.org> wrote:
The restriction of not checking the ownership of a directory is unnecessary given that all the package filelists contain this information. Remove this restriction, with the expectation that you might get multiple packages returned for a given directory. Additionally attempt to minimise the number of files getting through to the slow realpath call.
This combines ideas from two patches that have been around for a long time.
Original-work-by: Andrew Gregory <andrew.gregory.8@gmail.com> Original-work-by: Dan McGee <dan@archlinux.org> Signed-off-by: Allan McRae <allan@archlinux.org>
I actually decided to try tackling this again recently too. If you're interested, you can see the most recent incarnation here: http://bitbucket.org/andrewgregory/pacman/changesets/tip/directory_owner It gets rid of a lot of the basename/dirname stuff which I think makes it a lot easier to follow.
I had a quick look through your patch. It is quite a change to the code so there is not anything obvious for me to grab into this one.
How far would your patch be from being ready? If the answer is really soon, and Dan likes the look of your changes, then I am quite happy for "my" patch to be ignored.
Allan
Unfortunately, in removing the dirname/basename stuff I built in the assumption that the pkgfile paths will never include symlinks or anything else that needs to be resolved. I'm not sure why they would, but it's something we support at the moment. If everybody's ok with adding that assumption, then the answer is really soon. If not, we should go ahead with your patch. I do have a couple suggestions for improving it though, which I'll send along separately.
participants (2)
-
Allan McRae
-
Andrew Gregory