On 3 Aug 2022, at 13:04, Dag-Erling Smørgrav <d...@freebsd.org> wrote:
> 
> The branch main has been updated by des:
> 
> URL: 
> https://cgit.FreeBSD.org/src/commit/?id=36d67475f5497664d33c41c2f6745dcb30b0ec42
> 
> commit 36d67475f5497664d33c41c2f6745dcb30b0ec42
> Author:     Dag-Erling Smørgrav <d...@freebsd.org>
> AuthorDate: 2022-08-03 18:59:28 +0000
> Commit:     Dag-Erling Smørgrav <d...@freebsd.org>
> CommitDate: 2022-08-03 19:03:49 +0000
> 
>    xinstall: fix dounpriv logic, add tests

This is quite a poor commit message. What was wrong with it? Especially
when the diff is cluttered with reformatting. I cannot obviously see
any behavioural changes, just some changes from & to && that I don’t
believe technically matter, even if poor practice and not intended.

Jess

>    Sponsored by:   Klara, Inc.
>    MFC after:      1 week
> ---
> usr.bin/xinstall/tests/install_test.sh | 80 ++++++++++++++++++++++++++++++++++
> usr.bin/xinstall/xinstall.c            | 15 +++----
> 2 files changed, 87 insertions(+), 8 deletions(-)
> 
> diff --git a/usr.bin/xinstall/tests/install_test.sh 
> b/usr.bin/xinstall/tests/install_test.sh
> index c723a6e0d280..e95f40fc19e7 100755
> --- a/usr.bin/xinstall/tests/install_test.sh
> +++ b/usr.bin/xinstall/tests/install_test.sh
> @@ -404,6 +404,84 @@ symbolic_link_relative_absolute_common_body() {
>       fi
> }
> 
> +atf_test_case set_owner_group_mode
> +set_owner_group_mode_head() {
> +     atf_set "require.user" "root"
> +}
> +set_owner_group_mode_body() {
> +     local fu=65531 fg=65531
> +     local cu=65532 cg=65532
> +     local u="$(id -u)"
> +     local g="$(id -g)"
> +     local m=0755 cm=4444
> +     printf "test" >testf
> +     atf_check chown "$fu:$fg" testf
> +     atf_check chmod "$m" testf
> +
> +     atf_check install testf testc
> +     atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -o "$cu" testf testc
> +     atf_check_equal "$cu:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -g "$cg" testf testc
> +     atf_check_equal "$u:$cg:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -o "$cu" -g "$cg" testf testc
> +     atf_check_equal "$cu:$cg:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -m "$cm" testf testc
> +     atf_check_equal "$u:$g:10$cm" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -o "$cu" -m "$cm" testf testc
> +     atf_check_equal "$cu:$g:10$cm" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -g "$cg" -m "$cm" testf testc
> +     atf_check_equal "$u:$cg:10$cm" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -o "$cu" -g "$cg" -m "$cm" testf testc
> +     atf_check_equal "$cu:$cg:10$cm" "$(stat -f"%u:%g:%p" testc)"
> +}
> +
> +atf_test_case set_owner_group_mode_unpriv
> +set_owner_group_mode_unpriv_head() {
> +     atf_set "require.user" "root"
> +}
> +set_owner_group_mode_unpriv_body() {
> +     local fu=65531 fg=65531
> +     local cu=65532 cg=65532
> +     local u="$(id -u)"
> +     local g="$(id -g)"
> +     local m=0755 cm=4444 cM=0444
> +     printf "test" >testf
> +     atf_check chown "$fu:$fg" testf
> +     atf_check chmod "$m" testf
> +
> +     atf_check install -U testf testc
> +     atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -U -o "$cu" testf testc
> +     atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -U -g "$cg" testf testc
> +     atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -U -o "$cu" -g "$cg" testf testc
> +     atf_check_equal "$u:$g:10$m" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -U -m "$cm" testf testc
> +     atf_check_equal "$u:$g:10$cM" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -U -o "$cu" -m "$cm" testf testc
> +     atf_check_equal "$u:$g:10$cM" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -U -g "$cg" -m "$cm" testf testc
> +     atf_check_equal "$u:$g:10$cM" "$(stat -f"%u:%g:%p" testc)"
> +
> +     atf_check install -U -o "$cu" -g "$cg" -m "$cm" testf testc
> +     atf_check_equal "$u:$g:10$cM" "$(stat -f"%u:%g:%p" testc)"
> +}
> +
> atf_init_test_cases() {
>       atf_add_test_case copy_to_nonexistent
>       atf_add_test_case copy_to_nonexistent_safe
> @@ -444,4 +522,6 @@ atf_init_test_cases() {
>       atf_add_test_case symbolic_link_relative_absolute_source_and_dest2
>       atf_add_test_case symbolic_link_relative_absolute_common
>       atf_add_test_case mkdir_simple
> +     atf_add_test_case set_owner_group_mode
> +     atf_add_test_case set_owner_group_mode_unpriv
> }
> diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c
> index 05b1444506db..ddad7ba9115e 100644
> --- a/usr.bin/xinstall/xinstall.c
> +++ b/usr.bin/xinstall/xinstall.c
> @@ -1009,19 +1009,18 @@ install(const char *from_name, const char *to_name, 
> u_long fset, u_int flags)
> #endif
>       }
> 
> -     if (!dounpriv & 
> -         (gid != (gid_t)-1 && gid != to_sb.st_gid) ||
> -         (uid != (uid_t)-1 && uid != to_sb.st_uid))
> +     if (!dounpriv && ((gid != (gid_t)-1 && gid != to_sb.st_gid) ||
> +         (uid != (uid_t)-1 && uid != to_sb.st_uid))) {
>               if (fchown(to_fd, uid, gid) == -1) {
>                       serrno = errno;
>                       (void)unlink(to_name);
>                       errno = serrno;
>                       err(EX_OSERR,"%s: chown/chgrp", to_name);
>               }
> -
> +     }
>       if (mode != (to_sb.st_mode & ALLPERMS)) {
>               if (fchmod(to_fd,
> -                  dounpriv ? mode & (S_IRWXU|S_IRWXG|S_IRWXO) : mode)) {
> +                 dounpriv ? mode & (S_IRWXU|S_IRWXG|S_IRWXO) : mode)) {
>                       serrno = errno;
>                       (void)unlink(to_name);
>                       errno = serrno;
> @@ -1036,7 +1035,7 @@ install(const char *from_name, const char *to_name, 
> u_long fset, u_int flags)
>        * trying to turn off UF_NODUMP.  If we're trying to set real flags,
>        * then warn if the fs doesn't support it, otherwise fail.
>        */
> -     if (!dounpriv & !devnull && (flags & SETFLAGS ||
> +     if (!dounpriv && !devnull && (flags & SETFLAGS ||
>           (from_sb.st_flags & ~UF_NODUMP) != to_sb.st_flags) &&
>           fchflags(to_fd,
>           flags & SETFLAGS ? fset : from_sb.st_flags & ~UF_NODUMP)) {
> @@ -1082,7 +1081,7 @@ compare(int from_fd, const char *from_name __unused, 
> size_t from_len,
>               return 1;
> 
>       do_digest = (digesttype != DIGEST_NONE && dresp != NULL &&
> -                     *dresp == NULL);
> +         *dresp == NULL);
>       if (from_len <= MAX_CMP_SIZE) {
>               if (do_digest)
>                       digest_init(&ctx);
> @@ -1390,7 +1389,7 @@ install_dir(char *path)
>                       ch = *p;
>                       *p = '\0';
> again:
> -                     if (stat(path, &sb) < 0) {
> +                     if (stat(path, &sb) != 0) {
>                               if (errno != ENOENT || tried_mkdir)
>                                       err(EX_OSERR, "stat %s", path);
>                               if (mkdir(path, 0755) < 0) {


Reply via email to