Re: [bug-inetutils] [PATCH 3/7] argp'fied argument parsing & tiny fixes

[Arash Yadegarnia]

On Sat, 2008-08-30 at 15:31 -0400, Alfred M. Szmidt wrote: 
> Thank you for working on this.
> 
>          * telnet/main.c: Include <argp.h> and
>            <libinetutils/libinetutils.h>.
>          Remove include for <getopt.h>.
>          (MAX_TLINE_BUF): New macro.
>          (cmd_args): New data structure.
>          (ARGP_PROGRAM_DATA): Call macro.
>          (argp, args_doc, doc, argp_options): New variables.
>          (parse_opt): New function.
>          (help, try_help, usage): Functions removed.
>          (long_options): Variables removed.
>          (args, argp): Variables removed.
>          (main): Use strncpy () instead of strcpy for copying command line
>          args. Use argp to parse program options. Moved all of secondary
>          argumetn parsing to parse_opt.
>            ^^^^^^^^
> Typo.
> 
> 
>    -/* Print a help message describing all options to STDOUT and exit with a
>    -   status of 0.  */
>    -static void
>    -help ()
>    +struct _cmd_args
>     {
>    -  fprintf (stdout, USAGE, prompt);
>    +  char *argv[8];
>    +  char **argptr;
>    +};
>    +typedef struct _cmd_args cmd_args;
> 
> _FOO tends to be reserved for the compiler, and lower level parts.  I
> generally use something like:
> 
> | struct cmd_args
> | {
> |   char **argv[8];
> |   char **argptr;
> | };
> | typedef struct cmd_args cmd_args_t;
> 
Ah, Actually this is my favorite style too.
Since I spotted some of the current code to use _FOO style, for sake of
following conventions I did it that way.
Anyways, Fixed. 
> 
> I don't follow the following code from parse_opt, what are you trying
> to achieve?
> 
>    +    case ARGP_KEY_ARG:
>    +      if (state->arg_num == 0)
>    +  /* More than 2 arguments */
>    +  if ((state->argc - state->next) > 1)
>    +    argp_usage (state);
>    +      
>    +      if (user)
>    +      {
>    +  *(c_args->argptr)++ = "-l";
>    +  *(c_args->argptr)++ = user;
>    +      }
>    +      if (family == 4)
>    +  *(c_args->argptr)++ = "-4";
>    +      else if (family == 6)
>    +  *(c_args->argptr)++ = "-6";
>    +
>    +      *(c_args->argptr)++ = arg;  /* host */
>    +      if (state->argc - state->next)
>    +  *(c_args->argptr)++ = state->argv[state->next]; /* port */
>    +
>    +      *(c_args->argptr) = NULL;
>    +
>    +      /* Froce argp to stop processing remaining args,
>    +       * as we've got them all. */
>    +      state->next = state->argc;
>    +      break;
>    +
>    +    default:
>    +      return ARGP_ERR_UNKNOWN;
>    +  }
>    +  
>    +  return 0;
>     }
> 

This is the way telnet program works. it has an outer layer (routines in
main() and also argument parsing routines) which is supposed to act as a
shell for its inner layer. (tn () routine in command.c, which does some
extra work to parse args passed from main() routine).

It's actually an ugly design and it seems to me that author has taken
portions of the code (command.c) from somewhere and then decided to
reuse it in some poor manner.

I _did_ tried to override this useless logic, but it was almost
impossible without changing a lot of code. I just tried to keep the old
behavior and logic in argp way :)

PS, telnet code looks to me as a real classic, mid 80's or something
like that. is it really that old ?


    * telnet/main.c: Include <argp.h> and <libinetutils/libinetutils.h>.
    Remove include for <getopt.h>.
    (MAX_TLINE_BUF): New macro.
    (cmd_args): New data structure.
    (ARGP_PROGRAM_DATA): Call macro.
    (argp, args_doc, doc, argp_options): New variables.
    (parse_opt): New function.
    (help, try_help, usage): Functions removed.
    (long_options): Variables removed.
    (args, argp): Variables removed.
    (main): Use strncpy () instead of strcpy for copying command line
    args. Use argp to parse program options. Moved all of secondary
    argument parsing to parse_opt.



Patch is updated.

telnet.patch
Description: Text Data