On 09/14/2013 11:24:42 PM, Isaac wrote:
On Sun, Sep 15, 2013 at 11:59:42AM +0900, Ashwini Sharma wrote:
> On Fri, Sep 13, 2013 at 11:31 PM, Isaac <ibid...@gmail.com> wrote:
> > I note that it appears to revert one of the recent changes to
> > lib/pending.c.
> >
> > (Would you mind sending new toys as just the *.c file that goes
in toys/*/
> > when it doesn't need to touch lib/?)
> my patch on lib/pending.c is only modifying get_int_value()
function.
> Checking '-' and leading white spaces in the string.
>
> I changed this
> - if (errno || numstr == ptr || *ptr || rvalue < lowrange ||
rvalue >
> highrange)
> - perror_exit("bad number '%s'", numstr);
>
> to
>
> + if(errno || *ptr) perror_exit("invalid number '%s'", numstr);
> + if(rvalue < lowrange || rvalue > highrange)
> + error_exit("out of range '%s'", numstr);
>
> for having a better error message.
>
See this patch in commit 1066 (manually munged to indicate intent...):
unsigned long rvalue = 0;
char *ptr;
- if(*numstr == '-' || *numstr == '+' || isspace(*numstr))
perror_exit("invalid number '%s'", numstr);
+
+ if (!isdigit(*numstr)) perror_exit("bad number '%s'", numstr);
errno = 0;
rvalue = strtoul(numstr, &ptr, 10);
- if(errno || numstr == ptr) perror_exit("invalid number '%s'",
numstr);
- if(*ptr) perror_exit("invalid number '%s'", numstr);
- if(rvalue >= lowrange && rvalue <= highrange) return rvalue;
- else {
- perror_exit("invalid number '%s'", numstr);
- return rvalue; //Not reachable; to avoid waring message.
- }
+ if (errno || numstr == ptr || *ptr || rvalue < lowrange ||
rvalue > highrange)
+ perror_exit("bad number '%s'", numstr);
+ return rvalue;
}
Rob's mentioned a few times that simpler messages are preferred;
http://landley.net/toybox/cleanup.html refers to this message:
http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000850.html
And that's part of the rationale listed in the change.
(however, I don't see how get_int_value can handle negative numbers;
rvalue and lowrange are unsigned longs...)
On a side note:
I'm considering addressing 32 bit hosts' inability to store command
line numerical argument values larger than 32 bits. (On 32 bit hosts,
"long" is 32 bit. So "truncate -s 8G file.img" doesn't work on a 32 bit
host.)
I see three potential approaches:
1) Make the implicit array at the start of GLOBALS always be 64 bit
values. This wastes space for pointers (which would ignore the last 4
bytes of the allocated space), and adds int64_t handling code on 32 bit
hosts, but doesn't add special case codepaths for 32 vs 64 bits. (on 64
bits, it was already 64 bits).
2) Teach the option parsing logic that not all entries are the same
size, and it needs to advance the pointer by sizeof(value). Not too
hard to do, but adds extra code to option parsing. (I could make that
code conditional on 32 bits only, but... ew.)
3) Do nothing and wait for 32 bit processors to go the way of 16 bit
processors. The new iPhone already has a 64 bit processor (because it's
a desktop replacement, as
http://www.cringely.com/2013/09/19/the-secret-of-ios-7/ goes into some
detail about).
So far I've been doing #2, but I'm thinking #2 might be worthwhile. Any
opinions?
If I were the one doing it, I might use "bad number" and "not in
range",
or maybe even for the latter (if it works! Haven't tested)
perror_exit("'%s' too '%s'", numstr, rvalue < lowrange ? "low" :
"high");
But that last might be a little more clever and less clear...
In general, s/invalid/bad/.
The lib code to parse these numbers is only worthwhile if lib/args.c is
using it. Otherwise we have two instances of the same basic
functionality, and I try to avoid that. (Otherwise subtle behavior
differences are inevitable; I hate to overuse the word "bug" but doing
things twice lets the two fall out of sync and contradictions crop up,
if there's only one whatever else happens its behavior will be
consistent with itself by default. "Wrong but consistent" still beats
"inconsistent" because it makes being wrong easier to find and deal
with.)
Many moons ago I pondered teaching the library parsing code to parse
arguments that WEREN'T arguments to flags, but were positional
parameters instead. At the time I decided it wasn't worth it. The case
I was dealing with was "mkdir -m MODE" having mode parsing logic, and
letting other things use that mode parsing logic. In the end I just
moved the mode parsing logic to lib/lib.c and had -m take a string, and
had main call the parsing function instead of doing it automatically
via the option string.
Also, the option parsing logic's numeric input parser always accepts
kilobyte, megabyte, gigabyte style suffixes and multiplies the result
accordingly. (My rationale for doing so is it would otherwise be an
invalid input that would cause the program to reject it, and it doesn't
require extra code.)
> IPv6 is in fact important. Working on it, but was confused whether
to have
> the IPv6 support
> in the same traceroute.c or separate. As far as I see, there will
be lot
> many if/else checks for IPv4/IPv6 cases.
> Like for the incoming packet parsing.
Same file. A separate IPv6 command is not desireable (Rob recenty
mentioned this somewhere--I think in the discussion of Strake's
mount or sed), though I could see a <name>6 OLDTOY() for those who
are accustomed to the two binaries...
I'm happy to alias the name and have the alternate name set a default
flag. I just don't want two parallel implementation of basically the
same code. (Even if there's a lot of if/else, there's bound to be
_some_ shared code.)
Rob
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
