Re: A problem, a fix and a request

[Derek Price]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Gregg Leichtman wrote:

| The "fix" is to replace the atomic rename operation with a
| non-atomic copy and unlink. Since this is happening inside the
| user's sandbox,
|
| I don't think an atomic operation is needed. I could be wrong.
|
| rename_file (entfilename, CVSADM_ENT); */
|
| copy_file (entfilename, CVSADM_ENT);                        //
| added by GSL if (unlink_file (entfilename) && !existence_error
| (errno))  // " error (0, errno, "unable to remove %s",
| entfilename); // "


Is this sufficient?  A rename over an existing file fails but a copy
over an existing file does not?

Also, assuming I am correct and this is not sufficient change, then
something like:

~    rename Entries Entries.Save
~    unlink Entries
~    copy Entries.Backup Entries
~    unlink Entries.Backup
~    unlink Entries.Save


and fail if step 1 above fails, and if steps 2 or 3 above fail, then:

~    rename Entries.Save Entries

In case that last step fails, to be as robust as possible, then the
read entries routines would need to become aware of what an
Entries.Save file meant and attempt to handle it.

I'm not convinced this complexity is justified.

| My request is to incorporate this change into the official code
| base, _IF_ I have not missed some fundamental implication of this
| change. I'm not sure why the rename needs to be atomic in this
| case, since my understanding is that only one person would work on
| a sandbox at a time. The code also indicates that the rename on
| some OS' is not atomic. Does anyone see a problem with this? If
| not, can it be incorporated?


Well, only one person generally works on a sandbox at a time, and it
is certainly the recommended approach, but there is nothing currently
prohibiting someone from running, for instance, a cvs update from one
terminal window and a cvs status from another.  As long as the rename
remains atomic, the second process to access the file cannot get a
partial read which looks like a corrupt or incomplete Entries file.

The problem is certainly in your Windows NFS server.  Since it should
be attempting to provide standard NFS semantics as much as possible to
NFS clients, I would presume that *it* should be smart enough to
attempt a rename && copy && unlink^2 when the rename fails.

It probably wouldn't be a major hardship to wrap rename with a
function that attempted the rename && copy && unlink^2 only when the
requested rename fails, and wouldn't remove the atomic behavior when
the possibility exists, but I am still reluctant for several reasons:

~   1. My own google search did not turn up other similar problem
~      descriptions.
~   2. This is almost certainly a non-standard setup and may contain
~      other problems which I'm not sure it is worth expending the time
~      and energy on once we get started.
~   3. This is a WOE32 NFS server bug.  (Though it may also exist in
~      UNIX SMB clients, I might argue that the bug would then exist in
~      the UNIX SMB client.)
~   4. Atomic renames in the repository are more important (if only for
~      co-access by RCS) and I'm not sure if there are other
~      implications for this wrapper.  It might need to be restricted
~      to use on the Entries file (or maybe CVS/* files), which makes
~      it fairly complex for a single purpose we do not yet officialy
~      support.


Regards,

Derek
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.0 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCNddLLD1OTBfyMaQRAob9AJ9PJyfpvvS2vC6MQqIOfNFAwLU6tQCeOak2
7LGU/Qk5J3iXOpvtrOFpaRg=
=YvQh
-----END PGP SIGNATURE-----