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.
Index: telnet/main.c
===================================================================
RCS file: /sources/inetutils/inetutils/telnet/main.c,v
retrieving revision 1.17
diff -u -p -r1.17 main.c
--- telnet/main.c 21 Oct 2006 18:08:45 -0000 1.17
+++ telnet/main.c 4 Sep 2008 18:41:18 -0000
@@ -30,11 +30,12 @@
#ifdef HAVE_CONFIG_H
# include <config.h>
#endif
+#include <libinetutils.h>
#include <sys/types.h>
-#include <getopt.h>
#include <stdlib.h>
+#include <argp.h>
#include "ring.h"
#include "externs.h"
@@ -45,6 +46,9 @@
#define OPTS_FORWARD_CREDS 0x00000002
#define OPTS_FORWARDABLE_CREDS 0x00000001
+/* Based on buffer size for 'tline' in tn3270.c */
+#define MAX_TLINE_BUF 200
+
#if 0
# define FORWARD
#endif
@@ -68,118 +72,268 @@ tninit ()
#endif
}
-#define USAGE "Usage: %s [OPTION...] [HOST [PORT]]\n"
-/* 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_t;
- puts ("Login to remote system HOST (optionally, on service port PORT)\n\n\
- -4, --ipv4 Use only IPv4\n\
- -6, --ipv6 Use only IPv6\n\
- -8, --binary Use an 8-bit data path\n\
- -a, --login Attempt automatic login\n\
- -c, --no-rc Don't read the user's .telnetrc file\n\
- -d, --debug Turn on debugging\n\
- -e CHAR, --escape=CHAR Use CHAR as an escape character\n\
- -E, --no-escape Use no escape character\n\
- -K, --no-login Don't automatically login to the remote system\n\
- -l USER, --user=USER Attempt automatic login as USER\n\
- -L, --binary-output Use an 8-bit data path for output only\n\
- -n FILE, --trace=FILE Record trace information into FILE\n\
- -r, --rlogin Use a user-interface similar to rlogin\n\
- -X ATYPE, --disable-auth=ATYPE Disable type ATYPE authentication");
-
-#ifdef ENCRYPTION
- puts ("\
- -x, --encrypt Encrypt the data stream, if possible");
-#endif
+ARGP_PROGRAM_DATA ("telnet", "2008", "FIXME unknown");
+
+const char args_doc[] = "[HOST [PORT]]";
+const char doc[] = "TELNET protocol client.";
+
+/* Define keys for long options that do not have short counterparts. */
+enum {
+ ARG_NOASYNCH = 256,
+ ARG_NOASYNCTTY,
+ ARG_NOASYNCNET
+};
+
+static struct argp_option argp_options[] = {
+#define GRP 0
+ {"debug", 'd', NULL, 0, "Turn on debugging", GRP+1},
+ {"ipv4", '4', NULL, 0, "Use only IPv4", GRP+1},
+ {"ipv6", '6', NULL, 0, "Use only IPv6", GRP+1},
+ {"binary", '8', NULL, 0, "Use an 8-bit data path", GRP+1},
+ {"login", 'a', NULL, 0, "Attempt automatic login", GRP+1},
+ {"no-rc", 'c', NULL, 0, "Don't read the user's .telnetrc file", GRP+1},
+ {"escape", 'e', "CHAR", 0, "Use CHAR as an escape character", GRP+1},
+ {"no-escape", 'E', NULL, 0, "Use no escape character", GRP+1},
+ {"no-login", 'K', NULL, 0, "Don't automatically login to the remote system",
+ GRP+1},
+ {"user", 'l', "USER", 0, "Attempt automatic login as USER", GRP+1},
+ {"binary-output", 'L', NULL, 0, "Use an 8-bit data path for output only",
+ GRP+1},
+ {"trace", 'n', "FILE", 0, "Record trace information into FILE", GRP+1},
+ {"rlogin", 'r', NULL, 0, "Use a user-interface similar to rlogin", GRP+1},
+ {"disable-auth", 'X', "ATYPE", 0, "Disable type ATYPE authentication", GRP+1},
+ {"encrypt", 'x', NULL, 0, "Encrypt the data stream, if possible", GRP+1},
+#undef GRP
+#define GRP 10
+ {NULL, 0, NULL, 0, "When using Kerberos authentication:", GRP},
+ {"fwd-credentials", 'f', NULL, 0, "Allow the the local credentials to be"
+ " forwarded", GRP+2},
+ {"realm", 'k', "REALM", 0, "Obtain tickets for the remote host in REALM"
+ " instead of the remote host's realm", GRP+2},
+#undef GRP
+#define GRP 20
+ {NULL, 0, NULL, 0, "TN3270 options:", GRP},
+ {"transcom", 't', "LINE", 0, "Encrypt the data stream, if possible", GRP+3},
+ {"noasynch", ARG_NOASYNCH, NULL, 0, NULL, GRP+3},
+ {"noasynctty", ARG_NOASYNCTTY, NULL, 0, NULL, GRP+3},
+ {"noasyncnet", ARG_NOASYNCNET, NULL, 0, NULL, GRP+3},
+#undef GRP
+ {NULL}
+};
+static error_t
+parse_opt (int key, char *arg, struct argp_state *state)
+{
+ static int family;
+ static char *user;
+ static cmd_args_t *c_args;
+
+ c_args = (cmd_args_t*) (state->input);
+
+ switch (key)
+ {
+ case '4':
+ family = 4;
+ break;
+
+ case '6':
+ family = 6;
+ break;
+
+ case '8':
+ eight = 3; /* binary output and input */
+ break;
+
+ case 'E':
+ rlogin = escape = _POSIX_VDISABLE;
+ break;
+
+ case 'K':
#ifdef AUTHENTICATION
- puts ("\n\
- When using Kerberos authentication:\n\
- -f, --fwd-credentials Allow the the local credentials to be forwarded\n\
- -k REALM, --realm=REALM Obtain tickets for the remote host in REALM\n\
- instead of the remote host's realm");
+ autologin = 0;
+#else
+ argp_error (state, "'-%c' is currently not supported on this machine.",
+ key);
#endif
+ break;
+
+ case 'L':
+ eight |= 2; /* binary output only */
+ break;
+
+ case 'X':
+#ifdef AUTHENTICATION
+ auth_disable_name (arg);
+#else
+ argp_error (state, "'-%c' is currently not supported on this machine.",
+ key);
+#endif
+ break;
+
+ case 'a':
+ autologin = 1;
+ break;
+
+ case 'c':
+ skiprc = 1;
+ break;
+
+ case 'd':
+ debug = 1;
+ break;
+
+ case 'e':
+ set_escape_char (arg);
+ break;
+
+ case 'f':
+#if defined(AUTHENTICATION) && defined(KRB5) && defined(FORWARD)
+ if (forward_flags & OPTS_FORWARD_CREDS)
+ argp_error (state, "%s: Only one of -f and -F allowed.\n", prompt);
+ forward_flags |= OPTS_FORWARD_CREDS;
+#else
+ argp_error (state, "'-%c' is currently not supported on this machine.",
+ key);
+#endif
+ break;
+ case 'F':
+#if defined(AUTHENTICATION) && defined(KRB5) && defined(FORWARD)
+ if (forward_flags & OPTS_FORWARD_CREDS)
+ argp_error (state, "%s: Only one of -f and -F allowed.\n", prompt);
+ forward_flags |= OPTS_FORWARD_CREDS;
+ forward_flags |= OPTS_FORWARDABLE_CREDS;
+#else
+ argp_error (state, "'-%c' is currently not supported on this machine.",
+ key);
+#endif
+ break;
+
+ case 'k':
+#if defined(AUTHENTICATION) && defined(KRB4)
+ {
+ extern char *dest_realm, dst_realm_buf[], dst_realm_sz;
+ dest_realm = dst_realm_buf;
+ strncpy (dest_realm, arg, dst_realm_sz);
+ }
+#else
+ argp_error (state, "'-%c' is currently not supported on this machine.",
+ key);
+#endif
+ break;
+
+ case 'l':
+ autologin = 1;
+ user = arg;
+ break;
+
+ case 'n':
+ SetNetTrace (arg);
+ break;
+
+ case ARG_NOASYNCH:
#if defined(TN3270) && defined(unix)
- puts ("\n\
- TN3270 options (note non-standard option syntax):\n\
- -noasynch\n\
- -noasynctty\n\
- -noasyncnet\n\
- -t LINE, --transcom=LINE");
+ noasynchtty = 1;
+ noasynchnet = 1;
+#else
+ argp_error (state, "'--noasynch' is currently not supported"
+ " on this machine.");
#endif
+ break;
-#if defined (ENCRYPTION) || defined (AUTHENTICATION) || defined (TN3270)
- putc ('\n', stdout);
+ case ARG_NOASYNCTTY:
+#if defined(TN3270) && defined(unix)
+ noasynchtty = 1;
+#else
+ argp_error (state, "'--noasynctty' is currently not supported"
+ " on this machine.");
#endif
+ break;
- puts ("\
- --help Give this help list\n\
- -V, --version Print program version");
+ case ARG_NOASYNCNET:
+#if defined(TN3270) && defined(unix)
+ noasynchnet = 1;
+#else
+ argp_error (state, "'--noasyncnet' is currently not supported"
+ " on this machine.");
+ break;
+#endif
- fprintf (stdout, "\nSubmit bug reports to %s.\n", PACKAGE_BUGREPORT);
+ case 'r':
+ rlogin = '~';
+ break;
- exit (0);
-}
+ case 't':
+#if defined(TN3270) && defined(unix)
+ transcom = tline;
+ strncpy (transcom, arg, MAX_TLINE_BUF);
+#else
+ argp_error (state, "'-%c' is currently not supported on this machine.",
+ key);
+#endif
+ break;
-/* Print a message saying to use --help to STDERR, and exit with a status of
- 1. */
-static void
-try_help ()
-{
- fprintf (stderr, "Try `%s --help' for more information.\n", prompt);
- exit (1);
-}
+ case 'x':
+#ifdef ENCRYPTION
+ encrypt_auto (1);
+ decrypt_auto (1);
+#else /* ENCRYPTION */
+ argp_error (state, "'-%c' is currently not supported on this machine.",
+ key);
+#endif /* ENCRYPTION */
+ break;
-/* Print a usage message to STDERR and exit with a status of 1. */
-static void
-usage ()
-{
- fprintf (stderr, USAGE, prompt);
- try_help ();
+ 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;
}
-static struct option long_options[] = {
- {"ipv4", no_argument, 0, '4'},
- {"ipv6", no_argument, 0, '6'},
- {"binary", no_argument, 0, '8'},
- {"login", no_argument, 0, 'a'},
- {"no-rc", no_argument, 0, 'c'},
- {"debug", no_argument, 0, 'd'},
- {"escape", required_argument, 0, 'e'},
- {"no-escape", no_argument, 0, 'E'},
- {"no-login", no_argument, 0, 'K'},
- {"user", required_argument, 0, 'l'},
- {"binary-output", no_argument, 0, 'L'},
- {"trace", required_argument, 0, 'n'},
- {"rlogin", no_argument, 0, 'r'},
- {"disable-auth", required_argument, 0, 'X'},
- {"encrypt", no_argument, 0, 'x'},
- {"fwd-credentials", no_argument, 0, 'f'},
- {"realm", required_argument, 0, 'k'},
- {"transcom", required_argument, 0, 't'},
- {"help", no_argument, 0, '&'},
- {"version", no_argument, 0, 'V'},
- {0}
-};
-
+static struct argp argp = {argp_options, parse_opt, args_doc, doc};
+
/*
* main. Parse arguments, invoke the protocol or command parser.
*/
int
main (int argc, char *argv[])
{
- extern char *optarg;
- extern int optind;
- int ch;
- int family = 0;
- char *user;
+ cmd_args_t c_args = {0};
#ifndef strrchr
char *strrchr ();
#endif
@@ -199,189 +353,23 @@ main (int argc, char *argv[])
else
prompt = argv[0];
- user = NULL;
-
rlogin = (strncmp (prompt, "rlog", 4) == 0) ? '~' : _POSIX_VDISABLE;
autologin = -1;
- while ((ch = getopt_long (argc, argv, "468EKLS:X:acde:fFk:l:n:rt:x",
- long_options, 0)) != EOF)
- {
- switch (ch)
- {
- case '4':
- family = 4;
- break;
-
- case '6':
- family = 6;
- break;
-
- case '8':
- eight = 3; /* binary output and input */
- break;
- case 'E':
- rlogin = escape = _POSIX_VDISABLE;
- break;
- case 'K':
-#ifdef AUTHENTICATION
- autologin = 0;
-#endif
- break;
- case 'L':
- eight |= 2; /* binary output only */
- break;
- case 'X':
-#ifdef AUTHENTICATION
- auth_disable_name (optarg);
-#endif
- break;
- case 'a':
- autologin = 1;
- break;
- case 'c':
- skiprc = 1;
- break;
- case 'd':
- debug = 1;
- break;
- case 'e':
- set_escape_char (optarg);
- break;
- case 'f':
-#if defined(AUTHENTICATION) && defined(KRB5) && defined(FORWARD)
- if (forward_flags & OPTS_FORWARD_CREDS)
- {
- fprintf (stderr,
- "%s: Only one of -f and -F allowed.\n", prompt);
- help (0);
- }
- forward_flags |= OPTS_FORWARD_CREDS;
-#else
- fprintf (stderr,
- "%s: Warning: -f ignored, no Kerberos V5 support.\n",
- prompt);
-#endif
- break;
- case 'F':
-#if defined(AUTHENTICATION) && defined(KRB5) && defined(FORWARD)
- if (forward_flags & OPTS_FORWARD_CREDS)
- {
- fprintf (stderr,
- "%s: Only one of -f and -F allowed.\n", prompt);
- help (0);
- }
- forward_flags |= OPTS_FORWARD_CREDS;
- forward_flags |= OPTS_FORWARDABLE_CREDS;
-#else
- fprintf (stderr,
- "%s: Warning: -F ignored, no Kerberos V5 support.\n",
- prompt);
-#endif
- break;
- case 'k':
-#if defined(AUTHENTICATION) && defined(KRB4)
- {
- extern char *dest_realm, dst_realm_buf[], dst_realm_sz;
- dest_realm = dst_realm_buf;
- strncpy (dest_realm, optarg, dst_realm_sz);
- }
-#else
- fprintf (stderr,
- "%s: Warning: -k ignored, no Kerberos V4 support.\n",
- prompt);
-#endif
- break;
- case 'l':
- autologin = 1;
- user = optarg;
- break;
- case 'n':
-#if defined(TN3270) && defined(unix)
- /* distinguish between "-n oasynch" and "-noasynch" */
- if (argv[optind - 1][0] == '-' && argv[optind - 1][1]
- == 'n' && argv[optind - 1][2] == 'o')
- {
- if (!strcmp (optarg, "oasynch"))
- {
- noasynchtty = 1;
- noasynchnet = 1;
- }
- else if (!strcmp (optarg, "oasynchtty"))
- noasynchtty = 1;
- else if (!strcmp (optarg, "oasynchnet"))
- noasynchnet = 1;
- }
- else
-#endif /* defined(TN3270) && defined(unix) */
- SetNetTrace (optarg);
- break;
- case 'r':
- rlogin = '~';
- break;
- case 't':
-#if defined(TN3270) && defined(unix)
- transcom = tline;
- strcpy (transcom, optarg);
-#else
- fprintf (stderr,
- "%s: Warning: -t ignored, no TN3270 support.\n", prompt);
-#endif
- break;
- case 'x':
-#ifdef ENCRYPTION
- encrypt_auto (1);
- decrypt_auto (1);
-#else /* ENCRYPTION */
- fprintf (stderr,
- "%s: Warning: -x ignored, no ENCRYPT support.\n", prompt);
-#endif /* ENCRYPTION */
- break;
+ c_args.argptr = c_args.argv;
+ *(c_args.argptr)++ = prompt;
+ /* Parse command line */
+ argp_parse (&argp, argc, argv, 0, NULL, &c_args);
- case '&':
- help ();
- case 'V':
- printf ("telnet (%s) %s\n", PACKAGE_NAME, PACKAGE_VERSION);
- exit (0);
-
- case '?':
- try_help ();
-
- default:
- usage ();
- }
- }
if (autologin == -1)
autologin = (rlogin == _POSIX_VDISABLE) ? 0 : 1;
- argc -= optind;
- argv += optind;
-
- if (argc)
+ /* We have command line args */
+ if ((c_args.argptr - c_args.argv) > 1)
{
- char *args[8], **argp = args;
-
- if (argc > 2)
- usage ();
- *argp++ = prompt;
- if (user)
- {
- *argp++ = "-l";
- *argp++ = user;
- }
- if (family == 4)
- *argp++ = "-4";
- else if (family == 6)
- *argp++ = "-6";
-
- *argp++ = argv[0]; /* host */
- if (argc > 1)
- *argp++ = argv[1]; /* port */
- *argp = 0;
-
if (setjmp (toplevel) != 0)
Exit (0);
- if (tn (argp - args, args) == 1)
+ if (tn (c_args.argptr - c_args.argv, c_args.argv) == 1)
return (0);
else
return (1);
_______________________________________________
bug-inetutils mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/bug-inetutils
