Re: ACL patch feedback

[Andreas Gruenbacher]

Jim,

> [to lurkers, I'm hoping finally to check in Andreas'
>  ACL patches soon.  http://www.suse.de/~agruen/coreutils/5.91/ ]

I'm taking the lurkers into CC now ;)

thank you very much for your feedback. Many of your comments relate only to 
the xattr patch I've sent in addition to the acl patches. My focus is on the 
acl patches first; let's do them before looking at the xattr patch, please. 
I'll keep your other comments for later.

On Thursday 24 November 2005 14:31, Jim Meyering wrote:
> -------------
> Please make sure that `make distcheck' (make syntax-check,
> in particular) passes.  There are a few minor snags.

A ``make syntax-check'' fails for me because there is no cvsu utility. I found 
a Perl version from somewhere on the net. Could you please make it so that 
``make syntax-check'' will also work with the tarballs, and make it so that 
it only uses commonly available tools? This would make it a lot simpler for 
me; a Google search reveals that others have asked for this before, too 
(http://lists.gnu.org/archive/html/bug-coreutils/2004-01/msg00068.html).

I now get this:

> ./m4/gettext.m4:  AC_REQUIRE([jm_AC_TYPE_LONG_LONG])dnl
> ./m4/gettext.m4:  AC_REQUIRE([jm_AC_HEADER_INTTYPES_H])
> ./m4/gettext.m4:  AC_REQUIRE([jm_AC_HEADER_STDINT_H])
> ./m4/gettext.m4:  AC_REQUIRE([jm_GLIBC21])dnl
> ./m4/gettext.m4:  AC_REQUIRE([jm_AC_TYPE_UINTMAX_T])dnl
> ./m4/glibc21.m4:AC_DEFUN([jm_GLIBC21],
> ./m4/inttypes_h.m4:AC_DEFUN([jm_AC_HEADER_INTTYPES_H],
> ./m4/longlong.m4:AC_DEFUN([jm_AC_TYPE_LONG_LONG],
> ./m4/stdint_h.m4:AC_DEFUN([jm_AC_HEADER_STDINT_H],
> ./m4/uintmax_t.m4:AC_DEFUN([jm_AC_TYPE_UINTMAX_T],
> ./m4/uintmax_t.m4:  AC_REQUIRE([jm_AC_HEADER_INTTYPES_H])
> ./m4/uintmax_t.m4:  AC_REQUIRE([jm_AC_HEADER_STDINT_H])
> ./m4/uintmax_t.m4:    AC_REQUIRE([jm_AC_TYPE_UNSIGNED_LONG_LONG])
> ./m4/ulonglong.m4:AC_DEFUN([jm_AC_TYPE_UNSIGNED_LONG_LONG],
> Makefile.maint: do not use jm_ in m4 macro names
> make: *** [sc_prohibit_jm_in_m4] Error 1

``Make distcheck'' fails because my gzip doesn't have the ``--rsyncable'' 
option. After removing this option, it screws up in strftime-check:

> --- strftime-check-src  2005-12-04 20:21:30.000000000 +0000
> +++ strftime-check-info 2005-12-04 20:21:31.000000000 +0000
> @@ -1,42 +1 @@
> -%
> -A
> -B
> -C
> -D
> -F
> -G
> -H
> -I
> -M
>  N
> -P
> -R
> -S
> -T
> -U
> -V
> -W
> -X
> -Y
> -Z
> -a
> -b
> -c
> -d
> -e
> -g
> -h
> -j
> -k
> -l
> -m
> -n
> -p
> -r
> -s
> -t
> -u
> -w
> -x
> -y
> -z
> make[2]: *** [strftime-check] Error 1
> make[2]: Leaving directory `/var/tmp/coreutils'
> make[1]: *** [distcheck-hook] Error 2
> make[1]: Leaving directory `/var/tmp/coreutils'
> make: *** [distcheck] Error 2

The acl tests pass when run by hand (see the minor attached fix though).

> -------------
> Some of your new functions (I'm looking at copy.c)
> lack comments describing what they do, and their inputs and outputs.
> Please add them.
>
> copy.c (copy_internal)
>   s/dst_mode_valid/restore_dst_mode/
>   declare it to be `bool', not int

Done.

> -------------
> In cp.c: why set new->mode when new->mode_valid is false?
>
>                   new->mode = stats.st_mode;
>                   new->mode_valid = 0;

Fixed. I've also renamed mode_valid to restore_mode like above.

> -------------
> In copy.c, #include "acl.h" in alphabetical order -- so
> before the other "*.h" headers.

Done.

> -------------
> In copy_internal, the new code adds a race condition:
>   call mkdir (dir, ...
>   lstat (dir, ... to see if we have S_IRWXU access,
>   if not, call chmod (dir, ... to add those permissions.

Well, the same pattern exists in other places the code, e.g., mknod followed 
by chown followed by chmod in copy_internal().

> Having just rewritten remove.c to be reentrant, I wondered
> if we could avoid the race using openat-style functions (lib/openat.c).
> Hmm... there seems to be no mkdirat-like function, even in Solaris --
> I expected openat to have an option to create directories, just as
> unlinkat has an option to make it work like rmdir.
> What we need is a mkdir-like function that returns a file descriptor
> open on the resulting directory.  It's feasible on systems with
> O_NOFOLLOW support.

A function that atomically changes into the new directory would work as well, 
as would setting the initial permissions to S_IRWXU only.

> Your change also removes the relatively new HAVE_FCHMOD code that
> uses fchmod to avoid a race condition.  I do see how that conflicted
> with adding ACL support, but wonder...  Do you know if there are any
> ACL interfaces (for doing things like acl_get_file and acl_set_file)
> that operate on file descriptors rather than on file names?

I have reworked that, and I'm now using file handle based acl operations where 
I can, too.

> We can save rewriting copy.c for another day.
> For now, I'd like to get ACL support in.

That's a long standing wish of mine, too ;)

The following patches are attached (apply in this order):

copy_reg_cleanup.diff

        Move some of the functionality duplicated in copy_reg and
        copy_internal in separate functions, and clean up the logic in
        copy_reg and copy_internal.

coreutils-acl.diff

        Dummy acl support, remove umask_kill, fix the logic for acls.

coreutils-acl+posix.diff

        POSIX 1003.1e draft 17 acl support. Now covers Linux and others
        like FreeBSD. I cannot test the non-Linux stuff very well, so
        there might be a few minor issues.

mv-acl-test.diff

        A minor test case fix.

Cheers,
Andreas.

-- 
Andreas Gruenbacher <address@hidden>
SUSE Labs, SUSE LINUX PRODUCTS GMBH / Novell Inc.

copy_reg_cleanup.diff
Description: Text Data

mv-acl-test.diff
Description: Text Data

coreutils-acl.diff
Description: Text Data

coreutils-acl+posix.diff
Description: Text Data