On Sun, Aug 30, 2015 at 9:15 PM, Rob Landley <[email protected]> wrote: > On 08/28/2015 08:02 PM, enh wrote: >> Fix mv on overwrite and its prompt. >> >> We need to remove the destination, not the source, to be able to overwrite. >> >> Also, coreutils mv doesn't prompt if it's not talking to a tty. This >> change also affects killall, crontab, find, and rm. The rm case at >> least is interesting --- coreutils silently *does* do the removal, >> whereas this patch would make toybox silently *not* do the removal. >> This despite the fact that coreutils rm on a tty defaults to 'n'. So >> do we want two different defaults for yesno? Or is this a coreutils >> bug? (It certainly seems surprising to me.) >> >> diff --git a/lib/lib.c b/lib/lib.c >> index c16cffe..85ea4e1 100644 >> --- a/lib/lib.c >> +++ b/lib/lib.c >> @@ -634,6 +634,8 @@ int yesno(char *prompt, int def) >> { >> char buf; >> >> + if (!isatty(0)) return def; >> + >> fprintf(stderr, "%s (%c/%c):", prompt, def ? 'Y' : 'y', def ? 'n' : 'N'); >> fflush(stderr); >> while (fread(&buf, 1, 1, stdin)) { > > Reasonable. > >> diff --git a/tests/mv.test b/tests/mv.test >> index 53fc999..030e9cc 100755 >> --- a/tests/mv.test >> +++ b/tests/mv.test >> @@ -96,3 +96,10 @@ touch file1 file2 >> testing "Move -n file new_file (exist)" "mv -n file1 file2 && >> [ -e file1 -a -e file2 ] && echo 'yes'" "yes\n" "" "" >> rm -f file* >> + >> +touch file1 file2 >> +chmod 400 file1 file2 >> +testing "Move file over unwritable file with no stdin" \ >> + "</dev/null mv file2 file1 && [ -e file -a ! -e file2 ] && echo 'yes'" \ >> + "yes\n" "" "" >> +rm -f file* > > Already checked in as part of earlier commit. > >> diff --git a/toys/posix/cp.c b/toys/posix/cp.c >> index d5e92f2..c9de14d 100644 >> --- a/toys/posix/cp.c >> +++ b/toys/posix/cp.c >> @@ -380,9 +380,10 @@ void cp_main(void) >> if (!stat(TT.destname, &st) >> && ((toys.optflags & FLAG_i) || !(st.st_mode & 0222))) >> { >> - fprintf(stderr, "%s: overwrite '%s'", toys.which->name, >> TT.destname); >> - if (!yesno("", 1)) rc = 0; >> - else unlink(src); >> + snprintf(toybuf, sizeof(toybuf), "%s: overwrite '%s'", >> + toys.which->name, TT.destname); >> + if (!yesno(toybuf, 1)) rc = 0; >> + else unlink(TT.destname); >> } >> } > > The reason I didn't do that (and the reason yesno() was initially > designed to let you print the start of the message yourself) is the name > isn't guaranteed to fit into toybuf. You're trimming to the > sizeof(toybuf) in the snprintf (I just did five minutes of digging > because the snprintf() man page is very unclear about whether the null > terminator is guaranteed to be written, but luckily the posix page on it > _is_ clear and it is in the nonzero size case). > > That said, truncating the filename is breakage. (Long path only shows > path, no idea what file we're referring to.) I'm intentionally doing an > openat() based approach to support arbitrarily deep filenames because > you can create them after the fact with directory mv so you simply can't > avoid 'em, and we just gotta cope. > > That said have an xmprintf() in lib/xwrap.c for exactly this purpose. > It's sad to churn malloc/free but the -i case is very much _not_ a > hotpath, so... (Or I could yank -i if !isatty(0) in main. :)
good point. an alternative would be to make yesno take a format string. i can send you a patch to do that if you'd like to go that route. (it would mean that the obvious code would then be correct even in long-path cases.) > But before I check any of that in... I have half a human_readable() > change in my lib/lib.c. Right, need to clean up all this half-finished > stuff in my workspace... > > Rob -- Elliott Hughes - http://who/enh - http://jessies.org/~enh/ Android native code/tools questions? Mail me/drop by/add me as a reviewer. _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
