On 11/03/2016 07:55 AM, Tom Lane wrote:
It appears to me that the problem is that commit 35484e98c broke the case of linking to an existing symlink.
That's right. Thanks for the diagnosis.
There's no reason to disallow that case
Unfortunately the 'link' system call has system-dependent behavior when linking to a symlink. Some systems follow the symlink, and others don't. For the systems that don't, hardlinking to a relative symlink can cause trouble, since the symlink is resolved relative to its parent directory, and this interpretation can differ in the source vs the destination directory. I fixed this in the attached patch by using linkat with AT_SYMLINK_FOLLOW, as this is immune to the problem of resolving symlinks and is the intended meaning here anyway. On older systems lacking linkat, the code checks whether the source is a symlink and refuses to hard-link to it; this should be good enough.
So a less error-prone solution would be to revert 35484e98c's changes to itsdir(), taking it back to doing only what the name suggests, and invent an itssymlink() function that has a similar API but tests for symlink-ness not directory-ness.
Thanks for this suggestion too. Done in the attached patch.
I would actually say this should be
- if (err != EEXIST && itsdir(name) < 0) { + if (err != EEXIST || itsdir(name) <= 0) {
as we really don't want the code to keep going unless there is a directory there.
Yes, that sounds right, and is also in the attached patch.
Maybe even drop the test on what the err code is, and only check itsdir()? If errno == EEXIST (a common case), there should be no need to check itsdir, as the callers of mkdir do not consider an existing non-directory to be trouble (they'll diagnose that themselves). So unless I'm missing something, I'm inclined to keep the err code test, as a minor optimization.
Thanks again for the diagnosis and suggestions. Please try the attached patch, which I've installed into the development repository.