Notes |
(0002681)
vda
08-24-07 07:22
|
> This is the error message you get:
> cp: cannot remove '/tmp/nonexistent/path/foo': No such file or directory
Which is true. cp couldn't create it. cp decided to try unlink,
and creating again. unlink didn't work either. cp lets you know that.
> What Linux's cp will say is:
> cp: cannot create regular file `/tmp/nonexistent/path/foo': No such file or directory
Read it again.
Do you really think that "I can't create 'foo', it doesn't exist" is better?
From naive point of view it is silly too: "of course it doesn't exist
before you create it!"
I undoubtedly can fix bbox cp to match GNU message, but code will be bigger
and GNU message is still semi-stupid.
For the record: fix should be here:
static int ask_and_unlink(const char *dest, int flags)
{
...
if (unlink(dest) < 0) {
===> if errno == ENOENT or ENOTDIR, explain that *path* is invalid, not filename <===
bb_perror_msg("cannot remove '%s'", dest);
return -1; // error
}
return 1; // ok (to try again)
} |
| |
(0002682)
kiltedknight
08-24-07 08:58
|
I've read it again. You don't seem to understand...
If the path in which you want to create the file does not exist, you should never see EEXIST... that implies the file already exists! |
| |
(0002683)
vda
08-24-07 09:55
|
bbox cp says:
cp: cannot remove '/tmp/nonexistent/path/foo': No such file or directory
which sounds stupid
GNU coreutils cp says:
cp: cannot create regular file `/tmp/nonexistent/path/foo': No such file or directory
which is also stupid.
Do you agree with me on the above?
If yes: then why we should change from one stupid message to another stupid one?
If no: explain why do you think that coreutils message is better. |
| |
(0002684)
kiltedknight
08-24-07 10:05
|
It's far less stupid to say, "No such file or directory" when part or all of the path does not exist than to say, "cannot remove..." because the user is trying to create a file, not remove one.
Also, try compiling with DO_POSIX_CP enabled as well... instead, you get the message that the file exists when you try to copy to a path that doesn't exist.
In all cases, the best choice of error messages is "No such file or directory," which will clue the user in on what is actually wrong. |
| |
(0002685)
vda
08-24-07 10:08
|
Re EEXIST:
} else if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
|| S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
|| S_ISLNK(source_stat.st_mode)
) {
// We are lazy here, a bit lax with races...
if (dest_exists) {
==> errno = EEXIST;
ovr = ask_and_unlink(dest, flags);
It is added here purely for the sake of bb_perror_msg inside ask_and_unlink in "#if DO_POSIX_CP" block to print correct error code. It will happen if you try to cp a device node, fifo, or symlink over existing file without -i and -f.
static int ask_and_unlink(const char *dest, int flags)
{
#if DO_POSIX_CP
if (!(flags & (FILEUTILS_FORCE|FILEUTILS_INTERACTIVE))) {
// Either it exists, or the *path* doesnt exist
==> bb_perror_msg("cannot create '%s'", dest);
return -1;
}
#endif
It will say "cp: cannot create 'foo': File already exists". |
| |
(0002686)
vda
08-24-07 13:52
|
kiltedknight, please test attached patch. It gives good error messages for both ENOENT and ENOTDIR:
$ ./busybox cp busybox /qwe/asd
cp: cannot create '/qwe/asd': Path does not exist
$ ./busybox cp busybox /vmlinuz/asd
cp: cannot stat '/vmlinuz/asd': Path has non-directory component
svn busybox:
$ ./busybox cp busybox /qwe/asd
cp: cannot remove '/qwe/asd': No such file or directory
$ ./busybox cp busybox /vmlinuz/asd
cp: cannot stat '/vmlinuz/asd': Not a directory
Cost:
ask_and_unlink 81 119 +38
cp_mv_stat2 79 99 +20 |
| |
(0002687)
kiltedknight
08-24-07 14:30
|
Patch is working. Thank you. :) |
| |
(0004444)
vda
02-13-08 08:52
|
Fixed in revision 21006. Thanks. |
| |
bb_cp_errmsg_fix.diff [^] (541 bytes) 08-23-07 10:55
