Eric Blake wrote:
> According to Jim Meyering on 10/1/2009 1:28 PM:
>>> Here's the current state of the series, finally ready for review. If we
>>> check it in as-is, then coreutils will have everything it needs to ensure
>>> consistent behavior of 'mv -T a b/' on every platform it already supports
>>> except for cygwin 1.5 (which has never been a show-stopper for coreutils
>>
>> I've skimmed through all of that.
>> Considering I spent only 20 minutes, I can't have
>> done it justice, but didn't spot any problems, either.
>
> I've completed these three additional patches to fix the outstanding
> issues, and am now doing a sanity check that my rebasing worked before
> pushing to savannah. Expect it soon.
Thanks!
A couple of minor suggestions below:
>>From 0eb9ddbf689cb195763848c52507685ebb07834d Mon Sep 17 00:00:00 2001
> From: Eric Blake <e...@byu.net>
> Date: Thu, 1 Oct 2009 11:57:47 -0600
> Subject: [PATCH 1/3] rename: fix another cygwin 1.5 bug
...
>>From 984235b49cd5dd2f2bcfe26a4988c1c8a2253b97 Mon Sep 17 00:00:00 2001
> From: Eric Blake <e...@byu.net>
> Date: Thu, 1 Oct 2009 15:31:32 -0600
> Subject: [PATCH 2/3] renameat: fix Solaris bugs
...
>>From 9b631e0590832e7a1739fe053c30c97fa4d1aa77 Mon Sep 17 00:00:00 2001
> From: Eric Blake <e...@byu.net>
> Date: Thu, 1 Oct 2009 16:46:08 -0600
> Subject: [PATCH 3/3] rename: fix mingw bugs
>
> Copy various workarounds from cygwin 1.5: rename("dir/.","name"),
> rename("dir","file"), rename("dir1","dir2"). Amazingly,
> even though mingw stat() has no way to identify hard linked
> files, and even though rename("hard1","hard2") destroys the
> hard link, the lower-level MoveFileEx does the right thing!
> So the only testsuite relaxation is for rename("dir","dir/sub")
> giving EACCES instead of EINVAL when "dir/sub" does not exist.
>
> * lib/rename.c (rpl_rename) [W32]: Fix trailing slash and
> directory overwrite bugs.
> * tests/test-rename.h (test_rename): Relax test.
...
> diff --git a/lib/rename.c b/lib/rename.c
...
> + /* Presence of a trailing slash requires directory semantics. If
> + the source does not exist, or if the destination cannot be turned
> + into a directory, give up now. Otherwise, strip trailing slashes
> + before calling rename. There are no symlinks on mingw, so stat
> + works instead of lstat. */
> + src_slash = ISSLASH (src[src_len - 1]);
> + dst_slash = ISSLASH (dst[dst_len - 1]);
> + if (stat (src, &src_st))
> + return -1;
> + if (stat (dst, &dst_st))
> + {
> + if (errno != ENOENT || (!S_ISDIR (src_st.st_mode) && dst_slash))
> + return -1;
> + dst_exists = false;
> + }
> + else
> + {
> + if (S_ISDIR (dst_st.st_mode) != S_ISDIR (src_st.st_mode))
> + {
The odd indentation highlights the fact there's a TAB on this line
and on the three following. If you use spaces, the code will
render readably regardless of quoting.
> + errno = S_ISDIR (dst_st.st_mode) ? EISDIR : ENOTDIR;
> + return -1;
> + }
> + dst_exists = true;
> + }
> +
> + /* There are no symlinks, so if a file existed with a trailing
> + slash, it must be a directory, and we don't have to worry about
> + stripping strip trailing slash. However, mingw refuses to
> + replace an existing empty directory, so we have to help it out.
> + And canonicalize_file_name is not yet ported to mingw; however,
> + for directories, getcwd works as a viable alternative. Ensure
> + that we can get back to where we started before using it. */
> + if (dst_exists && S_ISDIR (dst_st.st_mode))
> + {
> + char *cwd = getcwd (NULL, 0);
Don't you want to handle getcwd failure here?
Otherwise, the chdir below dereferences NULL.
> + char *src_temp;
> + char *dst_temp;
> + if (chdir (cwd))
> + return -1;
> + if (IS_ABSOLUTE_FILE_NAME (src))
> + {
> + dst_temp = chdir (dst) ? NULL : getcwd (NULL, 0);
> + src_temp = chdir (src) ? NULL : getcwd (NULL, 0);
> + }
> + else
> + {
> + src_temp = chdir (src) ? NULL : getcwd (NULL, 0);
> + if (!IS_ABSOLUTE_FILE_NAME (dst))
> + chdir (cwd);
This chdir may fail.
> + dst_temp = chdir (dst) ? NULL : getcwd (NULL, 0);
> + }
> + chdir (cwd);
Same here. Neither failure may be ignored.
> + free (cwd);
> + if (!src_temp || !dst_temp)
> + {
> + free (src_temp);
> + free (dst_temp);
> + errno = ENOMEM;
> + return -1;
> + }
> + src_len = strlen (src_temp);
> + if (strncmp (src_temp, dst_temp, src_len) == 0
> + && (ISSLASH (dst_temp[src_len]) || dst_temp[src_len] == '\0'))
> + {
> + error = dst_temp[src_len];
> + free (src_temp);
> + free (dst_temp);
> + if (error)
> + {
> + errno = EINVAL;
> + return -1;
> + }
> + return 0;
> + }
> + if (rmdir (dst))
> + {
> + error = errno;
> + free (src_temp);
> + free (dst_temp);
> + errno = error;
> + return -1;
> + }
> + free (src_temp);
> + free (dst_temp);
> + }
...
