[Toybox] [Patch - New Toy] Add traceroute

[Isaac]

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...)

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/.

> 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...
 
> Any suggestions are welcome.
> 
> regards,
> Ashwini

Thanks,
Isaac Dunham