ACL patch feedback

[Jim Meyering]

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

Andreas,

Thanks for your patience.
I know you wrote most of this patch a *long* time ago...

Here's some feedback:
-------------
Can you explain why is it useful to add a separate --attributes=REGEX
option to cp?  Why not just add `xattr' or something similar to the
list of keywords in a --preserve=... list and have that try to preserve
all of them?
-------------
Please make sure that `make distcheck' (make syntax-check,
in particular) passes.  There are a few minor snags.
-------------

m4/xattr.m4: AC_FUNC_XATTR: Please rename to gl_FUNC_XATTR.
The cache variables should have the gl_ prefix, too, not ac_.
-------------

In coreutils, the preferred way to require such macros is
via a line like this
  AC_REQUIRE([gl_FUNC_XATTR])
in m4/prereq.m4, not in configure.ac.
-------------

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

In cp.c: why set new->mode when new->mode_valid is false?

                    new->mode = stats.st_mode;
                    new->mode_valid = 0;
-------------

[assuming there's a good reason for --attributes=REGEX, ]
With the XATTR changes, copy.c's copy_extended_attributes
exits if it encounters an invalid xattr regexp.  It'd be better
not to do that.  cp and mv should continue processing
remaining command-line arguments even if there's a failure
with a preceding one.  I know there are other uses of functions
in that file (xmalloc, xreadlink) that can exit nonzero, but
they do so only when virtual memory is exhausted, so it'd be
good if you could fix this.
-------------

Looking at this

  static void
  copy_attr_free (struct error_context *ctx, const char *str)
  {
    free ((void *) str);
  }

I don't like casting away the `const'.
But I suppose the error_context type requires the
function signature you've used.  That's unfortunate.
-------------

Hmm... I see that this is `extern' in copy.c:

  struct copy_attr_context
    {
      struct error_context ctx;
      const char *re_pattern;
      struct re_pattern_buffer re_compiled;
    } copy_attr_ctx = {
      { copy_attr_error,
        copy_attr_quote,
        copy_attr_free }
    };

Since it is used only in copy_extended_attributes,
it should be declared local (and automatic) to that function.
So you'll have to initialize those three members separately.

The goal is that eventually the copy function will
be reentrant, so I try to avoid file-scoped variables.
-------------

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

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.

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.

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?

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