Alan Curry wrote:
> Марк Коренберг writes:
>>
>> How to reproduce:
>>
>> $ ln -s non-exist tgt
> [...]
>> $ cp /etc/passwd tgt/
>> cp: cannot create regular file `tgt/': Is a directory
>>
>> Novices can not understand this message :)
>
> The same confusing error message also occurs in the simpler case where the
> target is simply any nonexistent name with a trailing slash.
>
> $ ls -ld tgt
> ls: cannot access tgt: No such file or directory
> $ cp /etc/passwd tgt/
> cp: cannot create regular file `tgt/': Is a directory
>
> strace shows this:
>
> open("tgt/", O_WRONLY|O_CREAT|O_EXCL|O_LARGEFILE, 0600) = -1 EISDIR
> (Is a directory)
>
> which I think is just bad kernel behavior. There's no errno (among the
> classical errno values anyway) which completely expresses "you tried to creat
> something with a trailing slash", but I'd rather see ENOENT or ENOTDIR than
> EISDIR.
>
> When this open fails, nothing in sight "Is a directory". The problem is that
> tgt/ _would_ be a directory, but since it doesn't exist and isn't about to be
> created, "Is a directory" is an overstatement.
>
> cp should dodge this issue by never calling creat with a trailing slash. It
> could supply a meaningful error message instead of one that is derived from
> an errno.Thanks for the suggestions. Here's the patch I'm considering: [I first patched copy.c's copy_reg, included at end, but didn't like that as much; the core copying code should not be encumbered like that, and other users of copy.c are not affected. ] This demonstrates the new diagnostic: $ touch k && rm -rf no-such && ./cp k no-such/ ./cp: invalid destination: `no-such/' a destination with a trailing slash must refer to an existing directory [Exit 1] This patch currently tests for EISDIR (linux kernels). If others systems fail with a different errno value, I'll adjust. >From cfa35c82106a63658879703a245cad5e6d8b7055 Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Sun, 21 Nov 2010 05:54:55 +0100 Subject: [PATCH] cp: give a better diagnostic for a special type of invalid argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * src/cp.c (do_copy): Give a better diagnostic when cp's 2nd and final argument ends in a slash, but does not name a directory. Reported by Марк Коренберг and Alan Curry. * THANKS: Update. * tests/mv/trailing-slash: Add a test. --- THANKS | 1 + src/cp.c | 26 +++++++++++++++++++++++--- tests/mv/trailing-slash | 9 +++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/THANKS b/THANKS index 0123c32..9bd78c8 100644 --- a/THANKS +++ b/THANKS @@ -362,6 +362,7 @@ M. P. Suzuki [email protected] Maciej Kwapulinski [email protected] Manas Garg [email protected] Manfred Hollstein [email protected] +Марк Коренберг [email protected] Marc Boucher [email protected] Marc Haber [email protected] Marc Lehman [email protected] diff --git a/src/cp.c b/src/cp.c index 5b14f3a..770add0 100644 --- a/src/cp.c +++ b/src/cp.c @@ -610,9 +610,29 @@ do_copy (int n_files, char **file, const char *target_directory, if (2 <= n_files && target_directory_operand (file[n_files - 1], &sb, &new_dst)) target_directory = file[--n_files]; - else if (2 < n_files) - error (EXIT_FAILURE, 0, _("target %s is not a directory"), - quote (file[n_files - 1])); + else + { + char const *dst_name = file[n_files - 1]; + if (2 < n_files) + error (EXIT_FAILURE, 0, _("target %s is not a directory"), + quote (dst_name)); + + if ( ! x->recursive) + { + /* Give a better diagnostic when the 2nd and final argument + of a non-recursive cp command ends in a slash but does not + name a directory. Before, cp FILE no-such/ would evoke + this misleading diagnostic: + cp: cannot create regular file `no-such/': Is a directory + in spite of the fact that "no-such" does not exist. */ + size_t dst_len = strlen (dst_name); + if (dst_len && dst_name[dst_len - 1] == '/') + error (EXIT_FAILURE, 0, + _("invalid destination: %s\na destination with a " + "trailing slash must refer to an existing directory"), + quote (dst_name)); + } + } } if (target_directory) diff --git a/tests/mv/trailing-slash b/tests/mv/trailing-slash index b58c908..c94ebb0 100755 --- a/tests/mv/trailing-slash +++ b/tests/mv/trailing-slash @@ -50,4 +50,13 @@ done #touch a a2 #mv a a2/ && fail=1 +# Test for a cp-specific diagnostic introduced after coreutils-8.7: +printf '%s\n' \ + "cp: invalid destination: \`no-such/'" \ + "a destination with a trailing slash must refer to an existing directory" \ +> expected-err +touch b +cp b no-such/ 2> err && fail=1 +compare err expected-err || fail=1 + Exit $fail -- 1.7.3.2.765.g642a8 For reference, here's the patch I wrote and discarded. Yes, it's buggy/incomplete, since I hadn't added the test to detect a zero-length dst_name: as-is, it would compare dst_name[-1] == '/': diff --git a/src/copy.c b/src/copy.c index 07501df..285768c 100644 --- a/src/copy.c +++ b/src/copy.c @@ -572,6 +572,16 @@ copy_reg (char const *src_name, char const *dst_name, dst_mode & ~omitted_permissions); dest_errno = errno; + if (dest_desc < 0 && dest_errno == EISDIR + && dst_name[strlen (dst_name) - 1] == '/' && ! x->move_mode) + { + error (0, 0, _("a destination with a trailing slash must " + "refer to an existing directory %s"), + quote (dst_name)); + return_val = false; + goto close_src_desc; + } + /* When trying to copy through a dangling destination symlink, the above open fails with EEXIST. If that happens, and lstat'ing the DST_NAME shows that it is a symlink, then we
