Re: [bug-inetutils] [PATCH] ping/ping6 --deadline option

[Alfred M. Szmidt]

   Here [2] is a patch for --deadline/-w option, and need suggestions ?

The option name is not very good, I had to read the patch to see what
it actually does.  Please make sure that the short option have the
same behaviour as BSD ping.  I would suggest --timeout=N.  Also please
update the NEWS file, and the ping manual accordingly.

   --- a/ping/libping.c
   +++ b/ping/libping.c
   @@ -89,6 +89,7 @@ ping_init (int type, int ident)
      /* Make sure we use only 16 bits in this field, id for icmp is a u_short. 
 */
      p->ping_ident = ident & 0xFFFF;
      p->ping_cktab_size = PING_CKTABSIZE;
   +  gettimeofday (&p->ping_start_time, NULL);

I'd guard that around an if, makes it clear what it is used for.

      return p;
    }

   diff --git a/ping/ping.c b/ping/ping.c
   index b83332f..5ef8c4d 100644
   --- a/ping/ping.c
   +++ b/ping/ping.c
   @@ -67,6 +67,7 @@ size_t interval;
    size_t data_length = PING_DATALEN;
    unsigned options;
    unsigned long preload = 0;
   +int deadline = DEFAULT_DEADLINE;

Both the otion name and variable name are terrible since the word
deadline does not convey anything about that this just aborts ping
after N seconds.

    int (*ping_type) (char *hostname) = ping_echo;

    int (*decode_type (const char *arg)) (char *hostname);
   @@ -113,6 +114,7 @@ static struct argp_option argp_options[] = {
      {"ignore-routing", 'r', NULL, 0, "send directly to a host on an attached "
       "network", GRP+1},
      {"verbose", 'v', NULL, 0, "verbose output", GRP+1},
   +  {"deadline", 'w', "NUMBER", 0, "stop after NUMBER seconds", GRP+1},

Use N instead, that is clear enough what --timeout would take for
values, and isn't very verbose.

    #undef GRP
    #define GRP 20
      {NULL, 0, NULL, 0, "Options valid for --echo requests:", GRP},
   @@ -176,6 +178,12 @@ parse_opt (int key, char *arg, struct argp_state *state)
          options |= OPT_QUIET;
          break;

   +    case 'w':
   +      deadline = ping_cvt_number (arg, 0, 0);
   +      if (deadline > INT_MAX)
   +        error (EXIT_FAILURE, 0, "invalid deadline value (%s)", arg);
   +      break;
   +
        case 'R':
          options |= OPT_RROUTE;
          break;
   @@ -338,6 +346,7 @@ ping_run (PING * ping, int (*finish) ())
      while (!stop)
        {
          int n;
   +      double interval = 0.0;

          FD_ZERO (&fdset);
          FD_SET (ping->ping_fd, &fdset);
   @@ -370,11 +379,19 @@ ping_run (PING * ping, int (*finish) ())
           {
             if (ping_recv (ping) == 0)
               nresp++;
   +
   +      gettimeofday (&now, NULL);
             if (t == 0)
   +        t = &now;
   +
   +      if (deadline != DEFAULT_DEADLINE)
               {
   -          gettimeofday (&now, NULL);
   -          t = &now;
   +          tvsub(&now, &ping->ping_start_time);
   +          /* now here has difference*/
   +          if (now.tv_sec >= deadline)
   +            break;

Please add a check so that this is only done in the case when a user
uses this options.  The indentation is also messed up...

Same applies for ping6.