Re: [bug-inetutils] Argpifying ping.

[Sergey Poznyakoff]

Debarshi 'Rishi' Ray <address@hidden> wrote:

> I have been trying to replace getopt with argp in ping.c. Since I am
> new to both inetutils and argp, I am curious to know whether I am
> proceeding in the right direction or not.

Not quite so, although it is a good start:) Please see the notes below.
 
> +  {"", '', 0, OPTION_DOC, "Options controlling ICMP request types:"},

Two mistakes there: 1) '' will produce a compilation error (empty character
constant) and 2) OPTION_DOC is not the same as header group option,
which you want in this case. Use this instead:

     { NULL, 0, NULL, 0, "Options controlling ICMP request types:", GRP },

(see below for the definition of GRP)     

> +  {"echo", ICMP_ECHO, 0, 0, "Send ICMP_ECHO requests (default)"},

Do not use ICMP_* constants as short option identifiers, because this
can create conflicts with other options.  If an option does not have
a short variant, define a special key for it, whose value is greater
than 255 (so that it cannot be given from command line as a single
char), e.g.:

#define ARG_ECHO 256
.
.
.
  { "echo", ARG_ECHO, NULL, 0, "Send ICMP_ECHO requests (default)", GRP+1 },

> +  {"address*", ICMP_ADDRESS, 0, 0, "Send ICMP_ADDRESS packets"},

Do not use * in long option names. It will need to be escaped in shell.
Besides, this will create incompatibility with the previous version of
ping.

> +  {"interval", 'i', "N", 0, "Wait N seconds between sending each packet"},

Use descriptive names for option arguments, e.g.:

   { "interval", 'i', "NUMBER", 0,
     "Wait NUMBER seconds between sending each packet"},

> +  {"count", 'c', "N", 0, "Stop after sending N packets (default: %d)"},

Same as above. Besides, %d will produce undefined behavior at the best
or coredump at the worst. If you wish to print default values, you will
have to do this after help output.

> +  {"preload", 'l', "N", 0, "Send N packets as fast as possible before 
> falling into\n
> +    normal mode of behavior"},

Do not use \n in option descriptions. Argp will correctly format the
text depending on the actual screen width, so use:

 {"preload", 'l', "NUMBER", 0,
  "Send NUMBER of packets as fast as possible before falling into "
  "normal mode of behavior"},

Beside this, the proposed options structure ignores option sorting,
while attempting to introduce option group headers. By default,
before producing help or usage output, argp will sort all options
alphabetically by their long name (well, in fact the sorting rules are
far more complex, but it does not matter now). So, in effect you will
get all options and option group headers sorted and intermixed without
any logical order. To order options by groups, use `group' member (the
last member of struct argp_option). Its effect is as follows: all
options with the same `group' value will be grouped together and sorted
alphabetically within that group; the groups will appear in the output
in order of increasing `group'. This also implies that an option group
header must have `group' value less than that of the group it should
precede. I usually use special macro to define sorting.

All that being said, the correct option structure will be:

/* Define keys for long options that do not have short counterpart */
enum {
     ARG_ECHO=256,
     ARG_ADDRESS,
     ARG_TIMESTAMP,
     ARG_ROUTERDISCOVERY
};

static struct argp_option options[] = {
#define GRP 0
  { NULL, 0, NULL, 0, "Options controlling ICMP request types:", GRP},
  { "echo", ARG_ECHO, NULL, 0, "Send ICMP_ECHO requests (default)", GRP+1 },
  { "address", ARG_ADDRESS, NULL, 0, "Send ICMP_ADDRESS packets", GRP+1 },
  { "timestamp", ARG_TIMESTAMP, NULL, 0, "Send ICMP_TIMESTAMP packets", GRP+1},
  /* This option is not yet fully implemented, so mark it as hidden */
  { "router", ARG_ROUTERDISCOVERY, NULL, OPTION_HIDDEN,
     "Send ICMP_ROUTERDISCOVERY packets", GRP+1 },
#undef GRP

/* Begin next group. I use an interval of 10 to make sure there is
   enough space available in case we would need to insert an additional
   group between these two */
#define GRP 10     
  { NULL, 0, NULL, 0, "Options valid for all request types:", GRP },
  { "count", 'c', "NUMBER", 0, "Stop after sending N packets",
   GRP+1},
  { "debug", 'd', NULL, 0, "Set the SO_DEBUG option", GRP+1 },
  { "interval", 'i', "SECONDS", 0,
    "Wait given number of SECONDS between sending each packet", GRP+1 },
  { "numeric", 'n', NULL, 0, "Do not resolve host addresses", GRP+1 },
  { "ignore-routing", 'r', NULL, 0,
    "Send directly to a host on an attached network", GRP+1 },
  { "verbose", 'v', NULL, 0, "Verbose output", GRP + 1},
#undef GRP

#define GRP 20  
  { NULL, 0, NULL, 0, "Options valid for --echo requests:", GRP},
  { "flood", 'f', NULL, 0, "Flood ping", GRP+1},
  { "preload", 'l', "NUMBER", 0,
    "Send NUMBER packets as fast as possible before falling into normal
    "mode of behavior", GRP+1 },
  { "pattern", 'p', "PAT", 0, "Fill ICMP packet with given pattern (hex)",
     GRP+1},
  { "quiet", 'q', NULL, 0, "Quiet output", GRP+1},
  { "route", 'r', NULL, 0, "Record route", GRP+1},
  { "size", 's', "NUMBER", 0, "Set number of data octets to send", GRP+1},
  { NULL }
#undef GRP
};

Now, regarding the actual parsing of the arguments:

> +struct arguments
> +{
> +  char *args[1];

This is a suggestion rather than a bug: there's no use in
this structure here, you'd be better off by using global variables
as the current version of ping does. This is much simpler and
requires less changes to the code.

If, however, you'd prefer to stick with `struct arguments', than please
notice that parse_opt is called once for each argument in the command
line, so this code:

> +static error_t
> +parse_opt (int key, char *arg, struct argp_state *state)
> +{
> +  char *endptr;
> +  struct arguments *arguments = state->input;
> +
> +  arguments->patptr = NULL;
> +  arguments->size = PING_DATALEN;
> +  arguments->preload = 0;
> +  arguments->interval = 0;
> +  arguments->ping_type = ping_echo;

is actually destroying the effect of options --size, --preload,
--interval, --echo, --address, --timestamp and --router (most of the
ping options, anyway). If you need to initialize stat->input, do it
when key == ARGP_KEY_INIT, like this:

static error_t
parse_opt (int key, char *arg, struct argp_state *state)
{
  char *endptr;
  struct arguments *arguments = state->input;

  switch (key)
    {
    case ARGP_KEY_INIT:
      arguments->patptr = NULL;
      arguments->size = PING_DATALEN;
      arguments->preload = 0;
      arguments->interval = 0;
      arguments->ping_type = ping_echo;
      break;
      .
      .
      .

But anyway, I'd suggest to use globals instead.
      
> +    case 'i':
> +      arguments->interval = strtod (arg, &endptr);
> +      if (*endptr)
> +        {
> +          fprintf (stderr, "Invalid value (`%s' near `%s')\n",
> +                   arg, endptr);
> +          exit (1);

To handle incorrect options/option arguments, use argp_error function:

    case 'i':
      arguments->interval = strtod (arg, &endptr);
      if (*endptr)
          argp_error (state, "Invalid value (`%s' near `%s')",
                      arg, endptr);

(There's no need to add terminating newline, nor to call exit(),
as argp_error never returns) 

> +    case ARGP_KEY_NO_ARGS:
> +      argp_usage (state);
> +      break;
> +    case ARGP_KEY_ARG:

These two cases contradict themselves: if the parser function handles
ARGP_KEY_ARG it will never be passed ARGP_KEY_NO_ARGS.

In general, you've chosen the wrong method to handle non-option
arguments. The 5th argument to argp_parse is designed to handle this
case:

  int index;
  
  if (argp_parse (argp, argc, argv, 0, &index, NULL))
     exit(1);

  argv += index;
  argc -= index;

so that you won't have to change anything from ping.c:272 on.

To summarize:

static error_t
parse_opt (int key, char *arg, struct argp_state *state)
{
  double v;
  char *endptr;
  
  switch (key)
    {
    case 'c':
      count = ping_cvt_number (arg, 0, 1);
      break;

    case 'd':
      socket_type = SO_DEBUG;
      break;

    case 'r':
      socket_type = SO_DONTROUTE;
      break;

    case 'i':
      v = strtod (arg, &endptr);
      if (*endptr)
        argp_error (state, "Invalid value (`%s' near `%s')",
                    arg, endptr);
      options |= OPT_INTERVAL;
      interval = v * PING_PRECISION;
      if (!is_root && interval < MIN_USER_INTERVAL)
        argp_error (state, "Option value too small: %s", arg);
      break;

    case 'p':
      decode_pattern (arg, &pattern_len, pattern);
      patptr = pattern;
      break;

    case 's':
      size = ping_cvt_number (arg, PING_MAX_DATALEN, 1);
      break;

    case 'n':
      options |= OPT_NUMERIC;
      break;

    case 'q':
      options |= OPT_QUIET;
      break;

    case 'R':
      options |= OPT_RROUTE;
      break;

    case 'v':
      options |= OPT_VERBOSE;
      break;

    case 'l':
      preload = strtoul (arg, &endptr, 0);
      if (*endptr || preload > INT_MAX)
        argp_error(state, "invalid preload value (%s)", arg);
      break;

    case 'f':
      options |= OPT_FLOOD;
      break;

    case 't':
      ping_type = decode_type (arg);
      break;

    case ICMP_ECHO:
      ping_type = decode_type ("echo");
      break;

    case ICMP_TIMESTAMP:
      ping_type = decode_type ("timestamp");
      break;

    case ICMP_ADDRESS:
      ping_type = decode_type ("address");
      break;

    case ICMP_ROUTERDISCOVERY:
      ping_type = decode_type ("router");
      break;

    default:
      return ARGP_ERR_UNKNOWN;
    }
    return 0;
}

Then the only change to  main() will be replacing getopt_long
loop with the call to argp_parse.

Regards,
Sergey