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 <[email protected]>
> ---
>
> 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
