Re: [bug-inetutils] Argpifying ifconfig.

[Alfred M. Szmidt]

   diff -urNp inetutils/ifconfig/changeif.c inetutils-build/ifconfig/changeif.c
   --- inetutils/ifconfig/changeif.c    2006-10-21 20:54:20.000000000 +0530
   +++ inetutils-build/ifconfig/changeif.c      2007-04-06 01:00:44.000000000 
+0530
   @@ -1,6 +1,6 @@
    /* changeif.c -- change the configuration of a network interface

   -   Copyright (C) 2001, 2002 Free Software Foundation, Inc.
   +   Copyright (C) 2001, 2002, 2007 Free Software Foundation, Inc.

       Written by Marcus Brinkmann.

   @@ -50,13 +50,14 @@
      if (!err)                                                         \
        {                                                                       
\
          fprintf (stderr, "%s: `%s' is not a valid address\n",         \
   -           program_name, addr);                                     \
   +           program_invocation_short_name, addr);                    \
          return -1;                                                    \

Do we really want to use program_invocation_short_name here and not
program_invocation_name?  When listing a error, it is common to get
the filename of the failing program.  It is a minor issue, you don't
need to change it right now, the person who commits can do it or
whatever.

   diff -urNp inetutils/ifconfig/ifconfig.c inetutils-build/ifconfig/ifconfig.c
   --- inetutils/ifconfig/ifconfig.c    2006-10-21 20:54:20.000000000 +0530
   +++ inetutils-build/ifconfig/ifconfig.c      2007-04-06 00:52:19.000000000 
+0530
   @@ -60,14 +60,17 @@ main (int argc, char *argv[])
      int sfd;
      struct ifconfig *ifp;

   -  parse_opt (argc, argv);
   +  /* Parse command line */
   +  if (argp_parse (&argp, argc, argv, ARGP_IN_ORDER, NULL, NULL))
   +    exit (1);

      sfd = socket (AF_INET, SOCK_STREAM, 0);
      if (sfd < 0)
        {
          fprintf (stderr, "%s: socket error: %s\n",
   -           program_name, strerror (errno));
   -      exit (1);
   +           program_invocation_short_name,
   +               strerror (errno));
   +      exit (EXIT_FAILURE);
        }

While we currently do not print a `type --help for more info' message
here, maybe we should?  What do you think?


   diff -urNp inetutils/ifconfig/options.c inetutils-build/ifconfig/options.c
   --- inetutils/ifconfig/options.c     2006-10-21 20:54:20.000000000 +0530
   +++ inetutils-build/ifconfig/options.c       2007-04-06 01:50:58.000000000 
+0530
   @@ -44,14 +44,64 @@

    #include <sys/socket.h>
    #include <net/if.h>
   +#include "libinetutils.h"
    #include "ifconfig.h"

   +struct argp_state *state;
   +
   +ARGP_PROGRAM_DATA("ifconfig", "2007", "Marcus Brinkmann")

Missing semi-colon at the end of the macro (screws up indenting
tools).

   +
   +const char args_doc[] = "[SYSTEM OPTION...]";
   +const char doc[] = "Configure network interfaces.";
   +
   +/* Define keys for long options that do not have short counterparts. */

A miniscule nitpick, two spaces after periods, even if followed by a
closing paren.

   @@ -282,20 +242,15 @@ PARSE_OPT_SET_INT (metric, metric value,
         void parse_opt_set_af (struct ifconfig *ifp, char *af)
    {
      if (!ifp)
   -    {
   -      fprintf (stderr, "%s: no interface specified for address"
   -           " family `%s'\n", program_name, af);
   -      usage (EXIT_FAILURE);
   -    }
   +    argp_error (state, "no interface specified for address "
   +                "family `%s'", af);

      if (!strcasecmp (af, "inet"))
        ifp->af = AF_INET;
      else
   -    {
   -      fprintf (stderr, "%s: unknown address family `%s' for interface `%s'"
   -           " is not a number.\n", program_name, optarg, ifp->name);
   -      exit (EXIT_FAILURE);
   -    }
   +    argp_failure (state, EXIT_FAILURE, 0,
   +                  "unknown address family `%s' for interface `%s' "
   +                  "is not a number", af, ifp->name);

Ah, those changes make me so happy...


   --- inetutils/ifconfig/printif.c     2006-10-21 20:54:20.000000000 +0530
   +++ inetutils-build/ifconfig/printif.c       2007-04-06 01:00:54.000000000 
+0530
   @@ -451,7 +451,7 @@ fh_index (format_data_t form, int argc,
      if (indx == 0)
        {
          fprintf (stderr, "%s: No index number found for interface `%s': %s\n",
   -           program_name, form->name, strerror (errno));
   +               program_invocation_short_name, form->name, strerror (errno));
          exit (EXIT_FAILURE);
        }

Shouldn't argp_failure be used here?  Same deal with the code that
follows.

   diff -urNp inetutils/ifconfig/system/generic.c
   inetutils-build/ifconfig/system/generic.c
   --- inetutils/ifconfig/system/generic.c      2006-10-21 20:54:20.000000000 
+0530
   +++ inetutils-build/ifconfig/system/generic.c        2007-04-04
   20:47:03.000000000 +0530

   diff -urNp inetutils/ifconfig/system/linux.c
   inetutils-build/ifconfig/system/linux.c
   --- inetutils/ifconfig/system/linux.c        2006-10-21 20:54:20.000000000 
+0530
   +++ inetutils-build/ifconfig/system/linux.c  2007-04-06 01:47:53.000000000 
+0530
   @@ -351,7 +352,7 @@ system_fh_hwaddr (format_data_t form, in
      if (ioctl (form->sfd, SIOCGIFHWADDR, form->ifr) < 0)
        {
          fprintf (stderr, "%s: SIOCGIFHWADDR failed for interface `%s': %s\n",
   -           program_name, form->ifr->ifr_name, strerror (errno));
   +           program_invocation_short_name, form->ifr->ifr_name, strerror 
(errno));
          exit (EXIT_FAILURE);
        }

argp_failure?  Or even error?  Maybe we are trying to do to much in a
single patch...

Looks good.