On 02/01/15 at 09:09pm, Allan McRae wrote:

Adjusting permissions of directories in their install scripts results in warnings such as the following when the package update:

warning: directory ownership differs on /var/lib/postfix/ filesystem: 73:0 package: 0:0

If the package being installed is the only owner of the directory, update the directory's permissions to those in the package file.

This also allows directory permissions for a package to be changed directly in the PKGBUILD file rather that requiring adjustment in an install scriptlet.

Signed-off-by: Allan McRae <allan@archlinux.org> ---

This does require searching through all installed packages filelist to see if any other directories own this file, which does create a slowdown in these error cases. Once the ability to use symbolic names for owners is added, this code path will run only if there is an acutal error or in the rare case that directory permissions are genuinely being changed.

Packages can currently avoid this being run by reverting their directory permission changes in pre_upgrade().

I'd like to push this to maint if people agree. These "false" warnings are causing too many issues.

lib/libalpm/add.c | 61 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 15 deletions(-)

Constantly shuffling the owner between root and another user feels very hackish to me just to avoid false positives. Perhaps we should just remove the warning until we have a proper way for packages to set ownership. If we do go ahead and reset ownership, I think it should also respect noupgrade. Style nitpicks below.

diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 82461ee..60db22f 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -167,6 +167,20 @@ static int extract_db_file(alpm_handle_t *handle, struct archive *archive, return perform_extraction(handle, archive, entry, filename); }

+static int other_directory_owners(alpm_handle_t *handle, const char *entryname, const char *pkgname) {

Wrap at 80 columns and move opening brace to its own line.

+ alpm_list_t *packages = alpm_db_get_pkgcache(handle->db_local); + alpm_list_t *i; + + for(i = packages; i ; i = alpm_list_next(i)) { + if(alpm_filelist_contains(alpm_pkg_get_files(i->data), entryname) && + strcmp(((alpm_pkg_t *)(i->data))->name, pkgname) != 0) { + return 1; + } + } + + return 0; +} + static int extract_single_file(alpm_handle_t *handle, struct archive *archive, struct archive_entry *entry, alpm_pkg_t *newpkg, alpm_pkg_t *oldpkg) { @@ -232,25 +246,42 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,

/* case 6: existing dir, ignore it */ if(lsbuf.st_mode != entrymode) { - /* if filesystem perms are different than pkg perms, warn user */ mode_t mask = 07777; - _alpm_log(handle, ALPM_LOG_WARNING, _("directory permissions differ on %s\n" - "filesystem: %o package: %o\n"), filename, lsbuf.st_mode & mask, - entrymode & mask); - alpm_logaction(handle, ALPM_CALLER_PREFIX, - "warning: directory permissions differ on %s\n" - "filesystem: %o package: %o\n", filename, lsbuf.st_mode & mask, - entrymode & mask); + if(other_directory_owners(handle, entryname, newpkg->name) == 0) { + /* no other package owns this directory, "extract" with correct permissions */ + if(chmod(filename, entrymode) != 0) { + goto warn_permissions; + } + } else { +warn_permissions:

I would prefer this was written as a single condition: if(noupgrade || other_directory_owners(...) != 0 || chmod(...) != 0) { /* warn user */ }

+ /* if filesystem perms are different than pkg perms, warn user */ + _alpm_log(handle, ALPM_LOG_WARNING, _("directory permissions differ on %s\n" + "filesystem: %o package: %o\n"), filename, lsbuf.st_mode & mask, + entrymode & mask); + alpm_logaction(handle, ALPM_CALLER_PREFIX, + "warning: directory permissions differ on %s\n" + "filesystem: %o package: %o\n", filename, lsbuf.st_mode & mask, + entrymode & mask); + + } }

if((entryuid != lsbuf.st_uid) || (entrygid != lsbuf.st_gid)) { - _alpm_log(handle, ALPM_LOG_WARNING, _("directory ownership differs on %s\n" - "filesystem: %u:%u package: %u:%u\n"), filename, - lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); - alpm_logaction(handle, ALPM_CALLER_PREFIX, - "warning: directory ownership differs on %s\n" - "filesystem: %u:%u package: %u:%u\n", filename, - lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + if(other_directory_owners(handle, entryname, newpkg->name) == 0) { + /* no other package owns this directory, "extract" with correct owners */ + if(chown(filename, entryuid, entrygid) != 0) { + goto warn_owners; + } + } else { +warn_owners:

Same as above.

+ _alpm_log(handle, ALPM_LOG_WARNING, _("directory ownership differs on %s\n" + "filesystem: %u:%u package: %u:%u\n"), filename, + lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + alpm_logaction(handle, ALPM_CALLER_PREFIX, + "warning: directory ownership differs on %s\n" + "filesystem: %u:%u package: %u:%u\n", filename, + lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + } }

_alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping dir extraction of %s\n", -- 2.2.2