Bug 208627

I'm seeing the following results when using rsync 3.1.2 that comes with FreeNAS 9.10:

$ touch a.txt

$ rsync -i -aX --acls a.txt b.txt
>f+++++++++ a.txt

$ rsync -i -aX --acls a.txt b.txt
.f.......a. a.txt

$ rsync -i -aX --acls a.txt b.txt
.f.......a. a.txt

$ getfacl a.txt
# file: a.txt
# owner: root
# group: user
            owner@:rw-p--aARWcCos:------:allow
            group@:r-----a-R-c--s:------:allow
         everyone@:r-----a-R-c--s:------:allow

$ getfacl b.txt
# file: b.txt
# owner: root
# group: user
            owner@:rw-p--aARWcCos:------:allow
            group@:r-----a-R-c--s:------:allow
         everyone@:r-----a-R-c--s:------:allow

Each run of with "--acls" considers the ACLs to have changed even if they haven't. Rsync built from ports using the standard options (i.e. with the ACL option unset) doesn't have this bug. Code inspection suggests that the problem is likely caused by "extrapatch-acl" making "set_acl" unconditionally return "changed == 1".

Compare the code snippet from the patch:

    if (sxp->brand == SMB_ACL_BRAND_NFS4) {
        ndx = F_ACL(file);
        if (ndx >= 0 && (size_t)ndx < nfs4_acl_list.count) {
            nfs4_duo *duo_item = nfs4_acl_list.items;
            duo_item += ndx;
            changed = 1;

            if (!duo_item->sacl) {
                duo_item->sacl = acl_from_text(duo_item->nacl.nfs4_acl_text);
                if (!duo_item->sacl)
                    return -1;
            }

            if (!dry_run && fname) {
                if (sys_acl_set_file(fname, SMB_ACL_TYPE_NFS4, duo_item->sacl) < 0) {
                    rsyserr(FERROR_XFER, errno, "set_acl: sys_acl_set_file(%s, %s)",
                        fname, str_acl_type(SMB_ACL_TYPE_NFS4));
                    return -1;
                }

                return changed;
            }
        }
    }

With a code snippet from the rest of the code:

    ndx = F_ACL(file);
    if (ndx >= 0 && (size_t)ndx < access_acl_list.count) {
        acl_duo *duo_item = access_acl_list.items;
        duo_item += ndx;
        eq = sxp->acc_acl
          && rsync_acl_equal_enough(sxp->acc_acl, &duo_item->racl, new_mode);
        if (!eq) {
            changed = 1;
            if (!dry_run && fname
             && set_rsync_acl(fname, duo_item, SMB_ACL_TYPE_ACCESS,
                      sxp, new_mode) < 0)
                return -1;
        }
    }

Today's ACL patch code simply sets "changed", but it should probably check "nfs4_acl_equal(sxp->nfs4_acl, duo_item->nacl)" first. It might also be necessary to move the return out of the inner if-statement for things to behave nicely (it doesn't make sense to only terminate early for non-dry runs).