[Toybox] [PATCH] Make yesno printf-like.

[Rob Landley]

On 09/02/2015 10:09 AM, enh wrote:
> On Tue, Sep 1, 2015 at 11:26 PM, Rob Landley <rob at landley.net> wrote:
>> On 09/01/2015 08:35 PM, enh wrote:
>>> Make yesno printf-like.
>>
>> This just makes me uncomfortable that it's nonobvious enough somebody's
>> going to yesno(1, usersupplieddata);.
> 
> that seems unlikely, especially given that yesno doesn't supply the
> "?". (and you really ought to get out of the habit of being so tricky
> at the cost of readability all the time anyway!)
> 
> btw, though this distracts from my point and will probably only
> encourage you to stick with the status quo... toybox already blindly
> uses user-supplied strings as format strings. random example:
> 
> $ seq -f '%s' 1 3
> seq: format ‘%s’ has unknown %s directive
> 
> $ ./toybox seq -f '%s' 1 3
> Segmentation fault (core dumped)

That's a bug and I need to fix it.

Except... Grrr. No posix entry for this, and the LSB spec is crap as usual.

seq -f %d 1 3
seq -f %s 1 3
seq -f "" 1 3
seq -f %f 1 3
seq -f %e 1 3
seq -f %g 1 3
seq -f "boo %f yah" 1 3
seq -f "boo %f %f yah" 1 3
seq -f "% f" 1 3
seq -f "%*f" 1 3
seq -f "%-1.2f" 1 3
seq -f "%-1.2.3f" 1 3
seq -f "%1-f" 1 3
seq -f "%1 f" 1 3
seq -f "%+-f" 1 3
seq -f "%+ - f" 1 3
seq -f "%.2f" 1 3
seq -f "%3.f" 1 3
seq -f "%2..2f" 1 3
seq -f "%'.2f" 1 3

Ok, it looks like the sanitization pass is using:

1) search for one % (must exist in string)
2) skip all "+- "
3) skip all digits 0-9
4) skip at most one "."
5) skip all digits 0-9
6) ensure next char is [feg]

Now let's read man 3 printf and see what ELSE I forgot: accept %%
literal, the target is aAeEfFgG, flags are ['#-+ ]...

seq -f "%LF" 1 3

Really? Sigh. I don't think I'm allowing that one because it's INSANE.

Grrr.

Ok, committed an insanitize() function.

> the compiler will find these for you with -Wformat-nonliteral (but for
> cases like this where you do need to use user-supplied input and just
> need to sanitize it first, there's no clean way to silence the warning
> when you're done; i guess we could add an fprintf_unsafe and rely on
> human code review to prevent inappropriate use [which would leave us
> no worse off than we are today]).
> 
>> You never reprompt for yesno so printing arbitrarily complex prompt
>> could be the caller's job. That was intentional.
> 
> but yesno does output the "(y/N)" part,

Because it's indicating which one is the default. That's why it needs to
produce that output.

> so i'm not sure what you gain
> from this. do you have an example where this is needed?

Well there's already several cases where code was producing its own
prompt text before calling yesno(), and I was thinking of just moving
the prompt generation out of yesno() entirely so they wouldn't have to
pass a "" argument telling it _not_ to prompt.

>>> In addition to making most existing code slightly simpler, this will
>>> let us move "if (!isatty(0)) return def;" into yesno so we can avoid
>>> printing a prompt in non-interactive situations.
>>
>> Except you don't want to do that without a flag of its own:
>>
>> $ cp -a lib bloit
>> $ cat | rm -ri bloit | cat
>> rm: descend into directory ‘bloit’? y
>> rm: remove regular file ‘bloit/help.c’? y
>>
>> That's the ubuntu implementation, not mine, so if it's wrong it's at
>> least definitively wrong. :) Suppressing the prompt has to be done on a
>> per-case basis.

if (isatty(0)) is not a huge burden on the callers. Printing our own
prompt text isn't a huge burden either. Combining the print logic into
yesno() was a minor optimization but if the print logic has to be
nontrivial, I'd rather have the callers do it rather than complicate
yesno().

And it looks like skipping due to isatty(0) is abnormal, killall isn't
doing it:

$ sleep 10000 &
$ sleep 10000 &
$ sleep 10000 &
$ killall -i sleep < /dev/null
Kill sleep(12602) ? (y/N) Kill sleep(12605) ? (y/N) Kill sleep(12606) ?
(y/N) sleep: no process found

find isn't doing it:

$ find toys.h -ok echo '{}' ';' < /dev/null
< echo ... toys.h > ?

(Again, ubuntu versions...)

And actually, posix doesn't say anything about stdin being a tty for -i,
the text from the 2008 spec is:

  If the -i option is in effect, the cp utility shall write a prompt to
  the standard error and read a line from the standard input. If the
  response is not affirmative, cp shall do nothing more with
  source_file and go on to any remaining files.

Meaning "yes | cp -i" should work...? (And cp -i default to 'n'.)

Lemme go back and look at your original message again to see what
problem you're trying to solve here...

Rob

 1441241898.0