bug#5956: [PATCH] cp: preserve "capabilities" when also preserving file ownership

[Jim Meyering]

Pádraig Brady wrote:
> `sudo cp -a non-root-file copy` would not preserve capabilities.
> The attached fixes this and passes all tests.
...
> Subject: [PATCH] cp: preserve "capabilities" when also preserving file 
> ownership
>
> * src/copy.c (copy_reg): Copy xattrs _after_ setting file ownership
> so that capabilities are not cleared when setting ownership.
> * tests/cp/capability: A new root test.
> * tests/Makefile.am (root_tests): Reference the new test.
> * NEWS: Mention the fix.

Good catch!
The patch looks fine.
Some tiny suggestions:

> diff --git a/NEWS b/NEWS
...
> +  cp now preserves "capabilities" when also preserving file ownership.

s/when also/also when/

>    ls --color once again honors the 'NORMAL' dircolors directive.
>    [bug introduced in coreutils-6.11]
>
> diff --git a/src/copy.c b/src/copy.c
> index 0fa148e..4e70c21 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -826,6 +826,22 @@ copy_reg (char const *src_name, char const *dst_name,
>          }
>      }
>
> +  /* We set ownership before xattrs as changing owners will
> +     clear capabilities.  */

Please use an active/imperative voice:

    /* Set ownership before setting xattrs, since setting ownership
       clears capabilities.  */

> +  if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb))
> +    {
> +      switch (set_owner (x, dst_name, dest_desc, src_sb, *new_dst, &sb))

...
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index db1610d..a943ff3 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -23,6 +23,7 @@ root_tests =                                        \
>    cp/preserve-gid                            \
>    cp/special-bits                            \
>    cp/cp-mv-enotsup-xattr                     \
> +  cp/capability                                      \
>    dd/skip-seek-past-dev                              \
>    install/install-C-root                     \
>    ls/capability                                      \
> diff --git a/tests/cp/capability b/tests/cp/capability
...
> +(setcap --help) 2>&1 |grep 'usage: setcap' > /dev/null \
> +  || skip_test_ "setcap utility not found"
> +(getcap --help) 2>&1 |grep 'usage: getcap' > /dev/null \
> +  || skip_test_ "getcap utility not found"
> +
> +# Don't let a different umask perturb the results.
> +umask 22

It's slightly better to use this function in place of the above two lines:

working_umask_or_skip_

> +touch file || framework_failure
> +chown $NON_ROOT_USERNAME file || framework_failure
...