>> Additions to the client/server protocol need to be discussed....
> actually there is not much changes to client/server protocol,

I believe your patch provides two new keywords with options be
sent from the client to the server. These appear to be

  1) "racl\012" with options -R, -l, -r <tag> and -- <files>
  2) "acl\012"  with options -R, -l, -r <tag> and -- <files>

The names need to be 'reserved' for use in both CVS and CVSNT
so that there are no-interoprability problems.

The presence of new REQ_LINE entries in the server are what
lets a CVS client know it is talking to a server which is
able to support the commands the client wishes to use.

Your client side code may need to use

    if (is_racl && !(supported_request ("racl"))
        error (1, 0, "server does not suport racl");

Or, you may wish to also check to see if the similar
CVSNT feature is supported using something like

        supported_request ("rlsacl")
or
        supported_request ("chacl2")

and adjust what you send to the server from the client to
be useful for the other implementation.

> it will just setuid when CVSServerRunAsUser keyword is set.

This is not really relevant to the client/server discussion,
but your are correct that setuid is also a potential issue.

>> It is highly desirable that you include doc/cvs.texi...
> which tool i should use to modify these documentation files?

I use either Emacs, XEmacs, or vim. They are simple text files,
so you should use your favorite text editor for the purpose.
You will probably want teTeX 3.0 or texinfo 4.8 if you wish
to convert the cvs.texi file into cvs.info and cvs.pdf files.

> Then i will edit them.

Okay.

>> I am concerned that this FEATURE seems to make use of a
>> set-uid or set-gid cvs executable ...

> I only use setuid when CVSServerRunAsUser keyword is set,

How safe is a set-uid or set-gid cvs executable? Running
in :pserver: mode, it switches out of root as quickly as
possible.

There are some folks that are using a set-gid cvs for some
things, but they typically do not make that cvs executable
available on an unprotected subnet. The NetBSD folks have
some additional SETXID_SUPPORT tests in their CVS code base,
but I have not had time to audit those changes for cvs 1.11.x
or look at the implications for cvs 1.12.x usage.

As such, it may be that you need your documentation to
recommend against using the CVSServerRunAsUser keyword
on unprotected networks... unless you are trying to say
that you have done a full audit of all of the set-uid and/or
set-gid code and know this is not a problem for anyone?

> it switch to specified user in server.c file,

This can potentially be a security hole, which is my concern.

> then other controls apply in server.c.
> So, i think it will not be a problem, ?

I do not actually trust CVS to run on the internet other than
using :ext: with an SSHv2 transport unless the server is a
read-only mirror of a safely protected repository.

> access file is normally under $CVSROOT. But when
> UseSeparateACLFileForEachDir keyword is set to
> yes, one access file is created for each directory
> in repository.

Okay. This is what I thought might be happening.

> I am not sure about this that is it needed or not.

It may indeed be needed, but control files for CVS
probably belong in a subdirectory called CVS
rather than at the same level as the ,v files.

This is one of the kinds of discussion that should
probably be thrashed out on the -email is unavailable-
mailing list.

> I thought about big repositories (thousands of
> files and directories and lots of acl definitions
> so a big access file...).

Yes, consistency locking of the file for read
could make a single CVSROOT/access file a hot-spot
and that is probably not desirable.

> When access file is very big, it may slow down cvs
> actions. Dont sure about it. ?

It seems likely that there will be a cost
associated with it.

> Actually, if access files under each directory
> are copied, acl definitions will also be copied,
> but i dont know cvsup will copy access files,
> will check it.

I suspect it is configurable, but I am not sure.
Thank you for looking into it. Your findings are
probably also reasonable to add to your
documentation.

This patch appears to remove some recent changes to the latest
sources even as it adds in your new feature. This is undesirable.

The indentation for the lines added via patch.diff does not
follow the HACKING guidelines. Indent by 4 spaces rather than
by a tabstop.

Additions to the client/server protocol need to be discussed
and ratified on both the -email is unavailable- and the
cvsnt-dev@march-hare.com. Please note that CVSNT already
has some ACL based commands (chacl2, rchacl2, lsacl, rlsacl).

In general, it would be nice to understand if there is any
functional overlap between CVSNT and your new proposed feature
addition to CVS.

It is highly desirable that you include doc/cvs.texi
(to add information on the new CVS commands and CVSROOT/config
options) and doc/cvsclient.texi (The CVS client/server protocol)

I am concerned that this FEATURE seems to make use of a
set-uid or set-gid cvs executable on the server. I believe
that the NetBSD folks have a number of SETXID_SUPPORT patches
to make this safer, but the use of the SETXID_SUPPORT setup
has not been well tested recently and may introduce security
flaws. Could you address the setups where ownership and groups
are being checked and modified? Is that only for :pserver: use
or is it also available under :ext: (just looking at the patch
it is not immediately obvious).

I do understand that this is an ongoing
  http://cvsacl.sourceforge.net
project and that it is desirable to get this code adopted
into the CVS FEATURE release. I just want to make sure
that everything needful is in place.


Please replace code constructs like this:

    accessfile = xmalloc (strlen (current_parsed_root->directory)
                  + sizeof (CVSROOTADM)
                  + sizeof (CVSROOTADM_ACCESS)
                  + 3);

    strcpy (accessfile, current_parsed_root->directory);
    strcat (accessfile, "/");
    strcat (accessfile, CVSROOTADM);
    strcat (accessfile, "/");
    strcat (accessfile, CVSROOTADM_ACCESS);

with Xasprintf () calls like this:

    accessfile = Xasprintf ("%s/%s/%s",
                            current_parsed_root->directory,
    CVSROOTADM, CVSROOTADM_ACCESS);

If I understand the code correctly, there are times when a
file called 'access' could be sprinkled throughout the
repository. I suspect that CVSup might not do the right thing
with such files (I have not looked closely). Do you know
if access rights are properly copied in such situations?

I suspect that the contrib/validate_repo.pl will need some
updates to deal with such files.

There may be more things that I missed, but that is my first
pass feedback on the patch so far.