Re: [bug-inetutils] Added new option --timeout-response to ping/ping6

[Alfred M. Szmidt]

   Alfred (or anyone interested :), may you review this one ?

Please format your code according to the GNU Coding Standards (spaces
between arithmetic operators, curlies on seperate lines and indented
accordingly, etc).

   +2009-10-22  Rakesh Pandit  <address@hidden>
   +
   +    * ping/ping_common.c (ping_set_timer): New function.
   +    * ping/ping_common.h (ping_set_timer): New prototype.

   +    * ping/ping6.c (argp_options): New option --timeout-response.
   +    (parse_opt): New option --timeout-response and check for edge
   +    values.

Seeing that it is a new option, there is no need to explain what is
being checked for.  For parse_opt, I'd write `Handle
--timeout-response', since the option is already mentioned as new, you
can't add two new options of the same name...

There is no entry for what you did in ping.c.

   +    (ping_run): Set timer for response timeout via ping_set_timer and
   +    corresponding signal for SIGALRM.

I'd write: Set signal action to SIGALRM for `sig_int', and handle
--timeout-response.


   diff --git a/doc/inetutils.texi b/doc/inetutils.texi
   index aba0c28..235505c 100644
   --- a/doc/inetutils.texi
   +++ b/doc/inetutils.texi
   @@ -474,6 +474,12 @@ the 8 bytes of ICMP header data.
    @opindex -w
    @opindex --timeout
    Stop after @var{n} seconds.
   +
   address@hidden -W @var{n}
   address@hidden address@hidden
   address@hidden -W
   address@hidden --timeout-response
   +Wait @var{n} seconds for response.

What happens after N seconds? That should be mentioned.

   @@ -182,6 +184,15 @@ parse_opt (int key, char *arg, struct argp_state *state)
          timeout = ping_cvt_number (arg, INT_MAX, 0);
          break;

   +    case 'W':
   +      timeout_response = strtoul (arg, &endptr, 0);
   +      if (*endptr || timeout_response < 0
   +          || timeout_response > INT_MAX/1000000) {
   +        error (EXIT_FAILURE, 0, "invalid response timeout value (%s)", arg);

GNU style error messages should be of the form `value: explanation',
so the above should be changed to `%s: invalid response timeout
value'.  E.g. `filename: no such file or directory'.

Is -W a standard option?

   +      }
   +      timeout_response *= 1000000;
   +      break;

There should be a small comment stating what this number means, maybe
make it a macro since its value is used in a few places.

   diff --git a/ping/ping6.c b/ping/ping6.c
   index a65b875..5924796 100644
   --- a/ping/ping6.c
   +++ b/ping/ping6.c
   @@ -55,6 +55,7 @@ size_t count = DEFAULT_PING_COUNT;
    size_t interval;
    int socket_type;
    int timeout = -1;
   +unsigned long timeout_response = -1;

Should be signed long if you wish to use -1.