[Toybox] [PATCH] Fix mv on overwrite.

[enh]

two patches here. the first patch is clearly a desirable bug fix but
there are some trickier compatibility choices lurking in the same
area. there's a bigger patch below, but i've provided both in case the
latter requires too much thought/experimentation :-)

(facebook found this bug:
https://code.google.com/p/android-developer-preview/issues/detail?id=3096)





Fix mv on overwrite.

We need to remove the destination, not the source, to be able to overwrite.

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*
diff --git a/toys/posix/cp.c b/toys/posix/cp.c
index d5e92f2..5a55f40 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -382,7 +382,7 @@ void cp_main(void)
{
fprintf(stderr, "%s: overwrite '%s'", toys.which->name, TT.destname);
if (!yesno("", 1)) rc = 0;
- else unlink(src);
+ else unlink(TT.destname);
}
}




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)) {
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*
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);
}
}

[Rob Landley]

Post by enh
two patches here. the first patch is clearly a desirable bug fix but

Er, yes. Sorry about that.

Post by enh
there are some trickier compatibility choices lurking in the same
area. there's a bigger patch below, but i've provided both in case the
latter requires too much thought/experimentation :-)

I read through so many weird rm corner cases making that work. And yes,
posix and gnu disagreed on some of them:

http://landley.net/notes-2012.html#06-12-2012

Oh, and the fix to the infinite directory traversal thing is to check
the stat info in the parent dirtree node when you traverse .. to go back
up and if it doesn't match traverse back down from the top as far as you
can confirm the matches for.

What you do about there being a significant gap between the two,
however, I'm honestly not sure about. (I need test cases. If somebody
just did a mv of your parent directory three levels up entirely out of
the tree you're considering, the behavior should still be correct,
essentially deferring the mv until after the traversal completes. If
somebody does a mv elsewhere within the same tree... that's pilot error,
and we avoid a rm or cp wandering outside the tree.)

I lean towards "if you have a significant gap, abort the darn directory
tree traversal because somebody did something stupid". Except rsync
should ignore it, because that's a best-effort thing on potentially
active filesystems. Maybe I need a DIRTREE_PERSEVERE flag? (And possibly
DIRTREE_INFINITE flag to not cache the filehandles, although that could
also be a dynamic "once you hit ~50 levels deep, switch modes" thing...
except we don't currently have global dirtree traversal state...)

My todo list has archaeological _layers_.

Post by enh
https://code.google.com/p/android-developer-preview/issues/detail?id=3096)

I apologize to facebook.

That report implies that Android M is frozen already. It looks like git
05499787ca89 is what you're shipping for M?

Post by enh
Fix mv on overwrite.
We need to remove the destination, not the source, to be able to overwrite.
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*
diff --git a/toys/posix/cp.c b/toys/posix/cp.c
index d5e92f2..5a55f40 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -382,7 +382,7 @@ void cp_main(void)
{
fprintf(stderr, "%s: overwrite '%s'", toys.which->name, TT.destname);
if (!yesno("", 1)) rc = 0;
- else unlink(src);
+ else unlink(TT.destname);
}
}

Applied.

I need to look at the second one more closely after dinner.

(I'm currently implementing the darn aboriginal linux toybox-test build
control image so I can run tests as root in a known environment, and
then I need to tackle the horrible state of the testsuite which is
failing all sorts of stuff and needs fixing. Then I need to finish the
nommu support because it's got my tree screwed up and not building
easily until that works through. And THEN I need to finish the cp
--preserve xargs support. And then I need to flush the other half-dozen
pending commits out of my tree...)

Thanks,

Rob

[enh]

Post by Rob Landley

Post by enh
two patches here. the first patch is clearly a desirable bug fix but

Er, yes. Sorry about that.

Post by enh
there are some trickier compatibility choices lurking in the same
area. there's a bigger patch below, but i've provided both in case the
latter requires too much thought/experimentation :-)

I read through so many weird rm corner cases making that work. And yes,
http://landley.net/notes-2012.html#06-12-2012
Oh, and the fix to the infinite directory traversal thing is to check
the stat info in the parent dirtree node when you traverse .. to go back
up and if it doesn't match traverse back down from the top as far as you
can confirm the matches for.
What you do about there being a significant gap between the two,
however, I'm honestly not sure about. (I need test cases. If somebody
just did a mv of your parent directory three levels up entirely out of
the tree you're considering, the behavior should still be correct,
essentially deferring the mv until after the traversal completes. If
somebody does a mv elsewhere within the same tree... that's pilot error,
and we avoid a rm or cp wandering outside the tree.)
I lean towards "if you have a significant gap, abort the darn directory
tree traversal because somebody did something stupid". Except rsync
should ignore it, because that's a best-effort thing on potentially
active filesystems. Maybe I need a DIRTREE_PERSEVERE flag? (And possibly
DIRTREE_INFINITE flag to not cache the filehandles, although that could
also be a dynamic "once you hit ~50 levels deep, switch modes" thing...
except we don't currently have global dirtree traversal state...)
My todo list has archaeological _layers_.

Post by enh
https://code.google.com/p/android-developer-preview/issues/detail?id=3096)

I apologize to facebook.
That report implies that Android M is frozen already. It looks like git
05499787ca89 is what you're shipping for M?

no, looks like this is the last upstream change merged into M:

commit 20019be7c8667b70ff68692b24029aed2c857639
Author: Rob Landley <***@landley.net>
Date: Thu May 14 13:48:55 2015 -0500

Bugfix from Hyejin Kim: su should not prompt root user for new
user's password.

at some point every release cycle, AOSP master no longer gets merged
into the release branch. it's still possible to cherrypick, but that
gets increasingly difficult as time passes. those of us on the lower
levels try to stabilize early. (this is also why i'm keen to get stuff
like uptime and lsof switched over now --- not because they have any
hope of being in M, but to maximize the testing they can get in master
before N ever opens for business.)

folks relying on toybox for automated testing should be shipping their
own known binary rather than relying on whatever's on the system, and
one day i'd like to get that into the NDK so it's easy for random
developers too.

Post by Rob Landley

Post by enh
Fix mv on overwrite.
We need to remove the destination, not the source, to be able to overwrite.
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*
diff --git a/toys/posix/cp.c b/toys/posix/cp.c
index d5e92f2..5a55f40 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -382,7 +382,7 @@ void cp_main(void)
{
fprintf(stderr, "%s: overwrite '%s'", toys.which->name, TT.destname);
if (!yesno("", 1)) rc = 0;
- else unlink(src);
+ else unlink(TT.destname);
}
}

Applied.
I need to look at the second one more closely after dinner.
(I'm currently implementing the darn aboriginal linux toybox-test build
control image so I can run tests as root in a known environment, and
then I need to tackle the horrible state of the testsuite which is
failing all sorts of stuff and needs fixing. Then I need to finish the
nommu support because it's got my tree screwed up and not building
easily until that works through. And THEN I need to finish the cp
--preserve xargs support. And then I need to flush the other half-dozen
pending commits out of my tree...)
Thanks,
Rob

[Rob Landley]

Post by Rob Landley

Post by enh
two patches here. the first patch is clearly a desirable bug fix but

Er, yes. Sorry about that.

Post by enh
there are some trickier compatibility choices lurking in the same
area. there's a bigger patch below, but i've provided both in case the
latter requires too much thought/experimentation :-)

I read through so many weird rm corner cases making that work. And yes,
http://landley.net/notes-2012.html#06-12-2012
Oh, and the fix to the infinite directory traversal thing is to check
the stat info in the parent dirtree node when you traverse .. to go back
up and if it doesn't match traverse back down from the top as far as you
can confirm the matches for.
What you do about there being a significant gap between the two,
however, I'm honestly not sure about. (I need test cases. If somebody
just did a mv of your parent directory three levels up entirely out of
the tree you're considering, the behavior should still be correct,
essentially deferring the mv until after the traversal completes. If
somebody does a mv elsewhere within the same tree... that's pilot error,
and we avoid a rm or cp wandering outside the tree.)
I lean towards "if you have a significant gap, abort the darn directory
tree traversal because somebody did something stupid". Except rsync
should ignore it, because that's a best-effort thing on potentially
active filesystems. Maybe I need a DIRTREE_PERSEVERE flag? (And possibly
DIRTREE_INFINITE flag to not cache the filehandles, although that could
also be a dynamic "once you hit ~50 levels deep, switch modes" thing...
except we don't currently have global dirtree traversal state...)
My todo list has archaeological _layers_.

Ok, not _quite_ that simple because symlinks.

When you follow a symlink (not the common case but we have a
DIRTREE_SYMFOLLOW knob), we need to cache the filehandle of the
symlink-used-as-directory and we can't discard that without traversing
all the way back down from the top because ".." doesn't return to where
we came from. Not the common case, but when I sat down to see if I could
easily implement this (and thus make rm -rf match this corner case of
the spec, and yes I'm waiting until I have the qemu based test
environment to _TEST_ that) I caught the hiccup.

The right thing to do would be to add a global maxfiles to struct
toy_context, have the call to dirtree_start() initialize it via
getrlimit(RLIMIT_NOFILE), and then have the DIRTREE_RECURSE path check
the returned filehandle and if it's greater than toys.maxfiles/2 close
parent->parent filehandles as necessary (not the one dirtree_parentfd()
would return but the one _above_ that) and then have the return path (or
more likely dirtree_parentfd()) reopen them via if (parent->data == -1)
openat(parent->data, ".."); and compare stat info and do the
retraverse-from-top dance as necessary. Module the cached symlink
handles thing to jump back there. (Actually I think their parent's fd is
what I need to save, the DIRTREE_SYMFOLLOW logic is doing
fstatat(AT_SYMLINK_FOLLOW) so their stat logic is directory match not
symlink so we can .. _to_ them, just not up past them. If we set closed
dir->data filehandles to -1 then the check is only on the descent side,
not on the return side. And since there's _already_ a symlink traversal
limit in the kernel for any given path (90 entries, I think?), running
out of filehandles due to symlink traversal with DIRTREE_SYMFOLLOW
enabled and erroring out because of it is find with me.)

Really the _hard_ part is updating the relevant section of code.html to
clearly explain how the new stuff would _work_. But I'd also want to
audit the users to make sure no code is using ->parent->data directly if
I've changed its semantics, although I think I already did that when
putting dirtree_parentfd() in?

Right now we use one filehandle per depth, not per node in the tree (we
open the directory filehandle when we recurse and close it again when we
return from it), and given the default 1024 rlimit this isn't a _common_
problem ("find . | sed 's/[^/]*//g' | sort -u" says the maximum depth in
the kernel source is 10 slashes, for example). But for the 1.0 release I
wanna do it right.

Meanwhile, sticking it back on the todo heap and poking at the
moundshalf-finished stuff instead of opening this new can of worms right
now. I need to _close_ tabs...

Rob

P.S. Yes I normally blog this nonsense instead of writing it here but
I'm a month and change behind on my blog updates. I have them
half-written up but not formatted properly and the links I want to
reference are mostly [TODO] notes, and so on.

[Rob Landley]

Post by enh
at some point every release cycle, AOSP master no longer gets merged
into the release branch. it's still possible to cherrypick, but that
gets increasingly difficult as time passes. those of us on the lower
levels try to stabilize early. (this is also why i'm keen to get stuff
like uptime and lsof switched over now --- not because they have any
hope of being in M, but to maximize the testing they can get in master
before N ever opens for business.)
folks relying on toybox for automated testing should be shipping their
own known binary rather than relying on whatever's on the system, and
one day i'd like to get that into the NDK so it's easy for random
developers too.

I hope that after the toybox 1.0 release the android base version is
generally usable for everybody.

It's... still a ways off. (Although I'm strongly tempted to start the
"vi" implementation since I know how to do it now, but... busy recanning
worms at present.)

Post by enh

Post by Rob Landley

Post by enh
Fix mv on overwrite.
We need to remove the destination, not the source, to be able to overwrite.
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*
diff --git a/toys/posix/cp.c b/toys/posix/cp.c
index d5e92f2..5a55f40 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -382,7 +382,7 @@ void cp_main(void)
{
fprintf(stderr, "%s: overwrite '%s'", toys.which->name, TT.destname);
if (!yesno("", 1)) rc = 0;
- else unlink(src);
+ else unlink(TT.destname);
}
}

Applied.
I need to look at the second one more closely after dinner.
(I'm currently implementing the darn aboriginal linux toybox-test build
control image so I can run tests as root in a known environment, and
then I need to tackle the horrible state of the testsuite which is
failing all sorts of stuff and needs fixing. Then I need to finish the
nommu support because it's got my tree screwed up and not building
easily until that works through. And THEN I need to finish the cp
--preserve xargs support. And then I need to flush the other half-dozen
pending commits out of my tree...)

Oh hey, I got distracted from that. I should get back to it and finish it...

Post by enh
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.

But _why_?

Post by enh
This
change also affects killall, crontab, find, and rm.

I tested rm, killall, and find and it didn't for me. They all prompted
and tried to read from stdin when < /dev/null.

The few times I've edited crontab I did so with vi because actual
multiuser systems are few and far between these days, although I suppose
the cloud might be bringing it back a little...

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/crontab.html

Does not mention -i option.

$ crontab -ir
no crontab for landley

Hmmm. Typing crontab by itself didn't give me my prompt back... Let's
see what the man page says:

The -l option causes the current crontab to be displayed on standard
output. See the note under DEBIAN SPECIFIC below.

No. Nope. Not going there. So very much not going DEBIAN SPECIFIC...

Sigh. What does busybox crontab --help say... no -i support. Of course not.

Post by enh
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.)

The default is an argument to yesno, it's context sensitive. That said,
what the right behavior is: open question. I lean towards not damaging
things when something goes wrong.

Post by enh
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;
+

The change I made instead yanked the prompt behavior from yesno() and
had the caller do it, meaning the !isatty(0) stuff would be the caller's
responsibility as well. (And there are times "yes | thingy-that-prompts"
is expected to fill in the y's, that's why "yes" is called that.)

Rob

[enh]

Post by Rob Landley

Post by enh
at some point every release cycle, AOSP master no longer gets merged
into the release branch. it's still possible to cherrypick, but that
gets increasingly difficult as time passes. those of us on the lower
levels try to stabilize early. (this is also why i'm keen to get stuff
like uptime and lsof switched over now --- not because they have any
hope of being in M, but to maximize the testing they can get in master
before N ever opens for business.)
folks relying on toybox for automated testing should be shipping their
own known binary rather than relying on whatever's on the system, and
one day i'd like to get that into the NDK so it's easy for random
developers too.

I hope that after the toybox 1.0 release the android base version is
generally usable for everybody.

it's a fine goal, but the whole reason facebook were poking around
this particular corner is because they were trying to find something
that worked on original flavor toolbox mv, BSD mv (which is what we'd
been shipping for a while), and toybox mv. so you don't just have to
reach a good point, and we don't just need to have shipped it --- we
need to wait until N or O or whichever release it is is so old that no
one cares about anything younger. so even if you're done tomorrow,
"use your own known binary" is going to be the best answer for these
kinds of folks.

but, yeah, one day toybox will be good enough and N will be
sufficiently far in the past that everyone's happy enough.

the only thing GNU has given me lately is to make grep more annoying
by turning "Is a directory" warnings on by default. other than that,
afaik no GNU tool has changed at all in the last decade. and i'd be
better off if i was still running an older grep.

Post by Rob Landley
It's... still a ways off. (Although I'm strongly tempted to start the
"vi" implementation since I know how to do it now, but... busy recanning
worms at present.)

Post by enh

Post by Rob Landley

Post by enh
Fix mv on overwrite.
We need to remove the destination, not the source, to be able to overwrite.
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*
diff --git a/toys/posix/cp.c b/toys/posix/cp.c
index d5e92f2..5a55f40 100644
--- a/toys/posix/cp.c
+++ b/toys/posix/cp.c
@@ -382,7 +382,7 @@ void cp_main(void)
{
fprintf(stderr, "%s: overwrite '%s'", toys.which->name, TT.destname);
if (!yesno("", 1)) rc = 0;
- else unlink(src);
+ else unlink(TT.destname);
}
}

Applied.
I need to look at the second one more closely after dinner.
(I'm currently implementing the darn aboriginal linux toybox-test build
control image so I can run tests as root in a known environment, and
then I need to tackle the horrible state of the testsuite which is
failing all sorts of stuff and needs fixing. Then I need to finish the
nommu support because it's got my tree screwed up and not building
easily until that works through. And THEN I need to finish the cp
--preserve xargs support. And then I need to flush the other half-dozen
pending commits out of my tree...)

Oh hey, I got distracted from that. I should get back to it and finish it...

Post by enh
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.

But _why_?

Post by enh
This
change also affects killall, crontab, find, and rm.

I tested rm, killall, and find and it didn't for me. They all prompted
and tried to read from stdin when < /dev/null.

yeah, i don't think i meant the GNU ones. i think i was pointing out
the choice between consistency and behave like the GNU tools.

personally i think without a strong argument to hide these prompts, it
makes more sense to show them. (though of course if stdin has been
closed, the chances are probably slim that stdout and stderr are going
anywhere useful. but at least we tried.)

Post by Rob Landley
The few times I've edited crontab I did so with vi because actual
multiuser systems are few and far between these days, although I suppose
the cloud might be bringing it back a little...
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/crontab.html
Does not mention -i option.
$ crontab -ir
no crontab for landley
Hmmm. Typing crontab by itself didn't give me my prompt back... Let's
The -l option causes the current crontab to be displayed on standard
output. See the note under DEBIAN SPECIFIC below.
No. Nope. Not going there. So very much not going DEBIAN SPECIFIC...
Sigh. What does busybox crontab --help say... no -i support. Of course not.

Post by enh
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.)

The default is an argument to yesno, it's context sensitive. That said,
what the right behavior is: open question. I lean towards not damaging
things when something goes wrong.

me too. that's why the coreutils rm behavior was surprising and seems
like a bug.

Post by Rob Landley

Post by enh
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;
+

The change I made instead yanked the prompt behavior from yesno() and
had the caller do it, meaning the !isatty(0) stuff would be the caller's
responsibility as well. (And there are times "yes | thingy-that-prompts"
is expected to fill in the y's, that's why "yes" is called that.)
Rob

[Rob Landley]

Post by enh
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.

Post by enh
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.

Post by enh
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. :)

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

[enh]

Post by Rob Landley

Post by enh
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.

Post by enh
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.

Post by enh
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.)

Post by Rob Landley
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

[enh]

Post by enh

Post by Rob Landley

Post by enh
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.

Post by enh
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.

Post by enh
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.)

i've sent a patch to make yesno printf-like because fixed-length
limits suck and existing uses of yesno often end up either formatting
into toybuf or writing to stderr themselves.

Post by enh

Post by Rob Landley
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.