bug-cvs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: cvs has problems using LockDir when CVSROOT holds a symlink


From: Mark D. Baushke
Subject: Re: cvs has problems using LockDir when CVSROOT holds a symlink
Date: Wed, 24 Sep 2003 00:48:38 -0700

Derek Robert Price <derek@ximbiot.com> writes:

> Mark D. Baushke wrote:
>
> | For 1.12.x, it might be reasonable to use the GNULIB version of
> |
> |canonicalize_file_name() by adding a lib/canonicalize.c file that
> |gets built if the function does not exist on the target platform.
>
> Is there a GNULIB version?

Yes. I see stdlib/canonicalize.c in glibc and I see the (more portable?)
coreutils project has the files:

    m4/canonicalize.m4
    lib/canonicalize.c
    lib/canonicalize.h

Taking a quick look inside of the m4 file I see the following:

        --------------- begin m4/canonicalize.m4 ---------------
#serial 1
AC_DEFUN([AC_FUNC_CANONICALIZE_FILE_NAME],
  [
    AC_REQUIRE([AC_HEADER_STDC])
    AC_CHECK_HEADERS(string.h sys/param.h stddef.h)
    AC_CHECK_FUNCS(resolvepath)
    AC_REQUIRE([AC_HEADER_STAT])

    # This would simply be AC_REPLACE_FUNC([canonicalize_file_name])
    # if the function name weren't so long.  Besides, I would rather
    # not have underscores in file names.
    AC_CHECK_FUNC([canonicalize_file_name], , [AC_LIBOBJ(canonicalize)])
  ])
         --------------- end m4/canonicalize.m4 ---------------

For URLs, here are what I found relating this this topic:

http://sources.redhat.com/cgi-bin/cvsweb.cgi/libc/stdlib/canonicalize.c?cvsroot=glibc
http://savannah.gnu.org/cgi-bin/viewcvs/coreutils/coreutils/lib/canonicalize.c
http://savannah.gnu.org/cgi-bin/viewcvs/coreutils/coreutils/lib/canonicalize.h
http://savannah.gnu.org/cgi-bin/viewcvs/coreutils/coreutils/m4/canonicalize.m4

I am not sure where you have been getting the GNULIB functions for the
lib and m4 files for our development version of cvs, so if this is the
wrong place, please let me know.

FWIW: I like canonicalize_file_name() better than the POSIX realpath()
function because the POSIX function is unable to deal with an unlimited
maximal path size. Hmmm.... It appears that the coreutils implementation
may use a sytem version of the realpath() function if available.

> I just looked and I do not see a canonicalize.c. If you meant to
> create a CVS lib version, I think I would prefer a name like
> canonicalize_path.c or the like.

Note the comment in m4/canonicalize.m4, they would rather not have
underscores in file names. This is one reason I suggested the
canonicalize.c name as well.

> I did just notice an interesting module in GNULIB named "same" that uses
> stat to compare two paths and verify that they point to the same inode,
> but that could be overkill in this case.  We couldn't use the "same"
> module directly regardless since it requires that the last component of
> the path (determined via basename) be the same string for some reason
> rather than using the inode there too.

So, appending '/.' to the pathname would be needed? Kind of ugly, but
I suppose it might work.

In any case, the lock_name() wants to take a full pathname to a
directory in the repository and determine what part of that name
is the same as the cvsroot and what is the module pathname so that
it can create a lock in the LockDir specified directory correctly.

So, I guess it would need to lop off successive elements from the
repository until it finds one with the same inode as the
current_parsed_root->directory ... that would be an approach that should
work and might even get around problems with the directory permissions
in the intervening directories making it difficult to find the path that
is really wanted. However, it seems like cvs is already relying on the
ability to have xgetwd() to return a full pathname without any problems.

> Also, my comments on issue #33 will be two years old on Friday.  That
> was long before I rewrote the code to handle symlinks and I ended up
> doing _that_ by rewriting do_recursion to keep track of recursing into
> directories for the r* commands rather than relying on xgetwd like I had
> in mind two years ago.

It is old commentary, but was a reasonable thing to say about the 1.11.x
code base.

> This sounds like a bug in that code, if it even exists at all anymore
> in 1.12.x. You didn't mention which version you found this problem in.
> As for 1.11.x, symlinks are pretty darn broke and I wouldn't bother
> fixing one single bug there anyhow. 

It would seem to be a matter of motivation... I found it in version
1.11.6 and opened issue #142 with it before I managed to find issue #33.
My problem is actually a subest of #33 in that.

> I had to overhaul quite a bit of code to get symlinks to work, which
> is why I only checked the fix into 1.12.x.

I understand.

In my case, I am doing work for a company that wants me to solve this
problem in the 1.11.x code base. While I could just give them a local
patch, it seems reasonable to me that the work should get put into
the cvshome source base so that a 1.11.7 version will not have this
particular problem. Yes, other problems may exist with symlinks, but
this is the one that is stopping them right now.

I verified that the problem was still present in 1.11.6.1 (cvs version).

I have verified that my patch to lock.c and root.c fixes the problem and
I have provided a test case that shows that the problem is fixed (to the
extent that the assert no longer fails and at least somewhat reasonable
output is provided for the r* commands).

> Sorry I misled you.  If you will confirm that this bug isn't in the
> 1.12.x tree, I will close issue #33.

The 1.12.x top-of-tree version of cvs does NOT exhibit the assert
failure when the LockDir directive is used in conjunction with a
CVSROOT that specifies a symbolic link.

It is also interesting that the RCS File: is 'more correct' in the
1.12.x version as it uses the symlink name rather than the canonical
name as a prefix in the path to the RCS,v filename.

You may close issue #33 as fixed in the 1.12 development version, but it
is still broken in the 'stable' version of cvs and it is difficult to
get folks to move off a 'stable' version of the release for production
use...

> I'm not sure whether it would be useful to commit your test case.  I
> hacked cvsroot at the beginning of sanity.sh to be a symlink and ran the
> entire suite when I first made symlinks work in 1.12.x and am making
> every test run with a symlinked root in addition to a standard one as
> part of the new autotest suite.

Excellent.

> Going through the ChangeLog, I also just noticed that I have already
> added an "xresolvepath" function to src/filesubr.c and the other
> locations.  It would probably be reasonable to move xresolvepath to
> lib/xresolvepath.c and use save_cwd and restore_cwd rather than
> reinventing the wheel like I appear to have in "xresolvepath".

I will note that xresolvepath() has an 'assert ( isdir (path) );' in it.
This is make the function less useful for the purpose I need in the
1.11.x version of parse_cvsroot() and local_cvsroot() the 'cvs init'
command passes in a directory that does not yet exist, so I would need
to duplicate the isdir (path) call before calling xresolvepath() to
avoid the assert, when getting a NULL return in that case is acceptable.

I am all for centralizing code that need not be duplicated and
maintained in multiple filesubr.c versions.

        Thanks,
        -- Mark

Attachment: pgpCkrpsfa_32.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]