Re: ACL patch feedback

[Jim Meyering]

Andreas Gruenbacher <address@hidden> wrote:
> 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.

Ok.

> 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

Ok.  I'll just include a copy of cvsu.
FYI, I added the following comment a few days ago:

  # cvsu is part of the cvsutils package: http://www.red-bean.com/cvsutils/

> ``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])
>> Makefile.maint: do not use jm_ in m4 macro names
>> make: *** [sc_prohibit_jm_in_m4] Error 1
...

You must have pulled in .m4 files from elsewhere.
When I run this command,
  grep -E 'jm_[A-Z]' m4/*.m4
I get no matches:

> ``Make distcheck'' fails because my gzip doesn't have the ``--rsyncable''

If you ever use rsync to transfer .gz files, then you should
start using the gzip patch to add that option.  It's well worth it.
rsync works well with modified .gz files, and the cost is minimal
(I think it was just a few percent for coreutils).

> option. After removing this option, it screws up in strftime-check:

These test failures are probably because of the old
.m4 files you used above.

>> --- 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
...
>> make[2]: *** [strftime-check] Error 1
>> make[2]: Leaving directory `/var/tmp/coreutils'
...
>> 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.

Thanks.  I'll look through those in the next few days.

>> 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):

Thanks again for persevering.
I'd really appreciate detailed ChangeLog entries.
I've gone ahead and written one for your copy_reg_cleanup.diff,
patch and have just checked that in.
It's not that different from your summary, but it does
name the new functions:

2005-12-05  Andreas Gruenbacher

        * src/copy.c [!HAVE_FCHOWN]: Define fchown(...) to -1.
        (set_owner, preserve_author): New functions, factored out of copy_reg.
        (copy_reg): Use them.
        (copy_internal): Use them here, too.

BTW, you'll notice that I've made some minor changes:
 - don't remove HAVE_FCHOWN or HAVE_FCHMOD definitions
 - use them instead of #if directives
 - tweak spacing

I notice that one of the remaining .diff files does contain ChangeLog
entries.  Please write ChangeLog entries for the others.

BTW, is there any reason not to name the preserve_author function
`set_author', so that it's consistent with `set_owner'?

> 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.