[Toybox] [Patch - New Toy] Add traceroute

[Rob Landley]

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.ag at 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