On Monday 2011-03-07 18:30, Stefan Tomanek wrote: > >I found the subcommand handling and naming done by iptables-multi and >ip6tables-multi very confusing and complicated; this patch reorganizes >the subcommands in a single table, allowing both variants of them to be >used (iptables/main) and also prints a list of the allowed commands if >an unknown command is entered by the user. I took the patch and already revised it for the points mentioned below. >diff --git a/ip6tables-multi.c b/ip6tables-multi.c >index 671558c..11fc6e7 100644 >--- a/ip6tables-multi.c >+++ b/ip6tables-multi.c >@@ -3,43 +3,23 @@ > #include <string.h> > #include <libgen.h> > >+#include "subcmd-multi.h" >+ There is xshared.h that can be used. >+subcmd_cb subcommands[] = { >+ { "ip6tables", &ip6tables_main }, >+ { "main", &ip6tables_main }, >+ { "ip6tables-save", &ip6tables_save_main }, >+ { "save", &ip6tables_save_main }, >+ { "ip6tables-restore", &ip6tables_restore_main }, >+ { "restore", &ip6tables_restore_main }, >+ { NULL, NULL } >+}; No typedefs for non-(function pointers). Also, & on the function pointer is undesired (it makes macro substitution - though not used here - a hassle). subcommands array be const. >index 0000000..3f3e914 >--- /dev/null >+++ b/subcmd-multi.h >@@ -0,0 +1,54 @@ >+#include <stdio.h> >+#include <stdlib.h> >+#include <string.h> >+#include <libgen.h> >+ >+typedef int(*mainfunc)(int, char**); >+typedef struct { char *name; mainfunc main; } subcmd_cb; >+ >+mainfunc get_subcommand(char *cmd, subcmd_cb cbs[]); >+int subcmd_main(int argc, char **argv, subcmd_cb cbs[]); Unnecessary prototypes (in case of subcmd-multi.h). >+ >+mainfunc get_subcommand(char *cmd, subcmd_cb cbs[]) Should be static because it is not used from anywhere but subcmd_main. cbs be const. >+{ Code does not belong in header files. Inconsistent mixed use of * and []. >+ int i = 0; >+ while ( cbs[i].name ) { >+ if ( strcmp(cmd, cbs[i].name) == 0 ) { >+ return cbs[i].main; >+ } >+ i++; >+ } Can be written much shorter by making cbs walk itself: for (; cbs->name != NULL; ++cbs) if (strcmp... return; >+ return NULL; >+} >+ >+int subcmd_main(int argc, char **argv, subcmd_cb cbs[]) >+{ >+ if (argc < 1) { >+ fprintf(stderr, "ERROR: This should not happen.\n"); >+ exit(EXIT_FAILURE); >+ } Unlikely to happen. >+ char *cmd = basename(argv[0]); >+ mainfunc f = get_subcommand(cmd, cbs); Incosistent naming. >+ /* Unable to find a main method for our command name? >+ * Let's try again with the first argument! >+ */ >+ if (! f && argc > 0) { Must be argc > 1 or it will run into undefined behavior when accessing argv[0]. >+ argv++; argc--; At least one line per statement. >+ cmd = argv[0]; >+ f = get_subcommand(cmd, cbs); >+ } >+ >+ /* now we should have a valid function pointer */ >+ if (f) { >+ return f(argc, argv); Else-considered-harmful. >+ } else { >+ fprintf(stderr, "ERROR: No valid subcommand given.\nValid subcommands:\n"); >+ int i = 0; >+ while ( cbs[i].name ) { >+ fprintf(stderr, " * %s\n", cbs[i].name); >+ i++; >+ } Using a walking pointer again. Authorship has been preserved. The finalized patch now looks like: parent f96cb8094ceffb9ffe8e94b4ee6800aa581dd021 (v1.4.10-87-gf96cb80) commit 9cc4f24e72f87ca191c2e723e7cd293f6477481c Author: Stefan Tomanek <stefan.tomanek@xxxxxxxxxxxxx> Date: Mon Mar 7 18:30:27 2011 +0100 ip(6)tables-multi: unify subcommand handling I found the subcommand handling and naming done by iptables-multi and ip6tables-multi very confusing and complicated; this patch reorganizes the subcommands in a single table, allowing both variants of them to be used (iptables/main) and also prints a list of the allowed commands if an unknown command is entered by the user. Signed-off-by: Jan Engelhardt <jengelh@xxxxxxxxxx> --- ip6tables-multi.c | 46 ++++++++++----------------------------- iptables-multi.c | 52 ++++++++++++-------------------------------- xshared.c | 36 +++++++++++++++++++++++++++++++ xshared.h | 11 +++++++++ 4 files changed, 73 insertions(+), 72 deletions(-) diff --git a/ip6tables-multi.c b/ip6tables-multi.c index 671558c..7e6603f 100644 --- a/ip6tables-multi.c +++ b/ip6tables-multi.c @@ -1,45 +1,23 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <libgen.h> +#include "xshared.h" int ip6tables_main(int argc, char **argv); int ip6tables_save_main(int argc, char **argv); int ip6tables_restore_main(int argc, char **argv); +static const struct subcommand multi6_subcommands[] = { + {"ip6tables", ip6tables_main}, + {"main", ip6tables_main}, + {"ip6tables-save", ip6tables_save_main}, + {"save", ip6tables_save_main}, + {"ip6tables-restore", ip6tables_restore_main}, + {"restore", ip6tables_restore_main}, + {NULL}, +}; + int main(int argc, char **argv) { - char *progname; - - if (argc < 1) { - fprintf(stderr, "ERROR: This should not happen.\n"); - exit(EXIT_FAILURE); - } - - progname = basename(argv[0]); - if (strcmp(progname, "ip6tables") == 0) - return ip6tables_main(argc, argv); - if (strcmp(progname, "ip6tables-save") == 0) - return ip6tables_save_main(argc, argv); - if (strcmp(progname, "ip6tables-restore") == 0) - return ip6tables_restore_main(argc, argv); - - ++argv; - --argc; - if (argc < 1) { - fprintf(stderr, "ERROR: No subcommand given.\n"); - exit(EXIT_FAILURE); - } - - progname = basename(argv[0]); - if (strcmp(progname, "main") == 0) - return ip6tables_main(argc, argv); - if (strcmp(progname, "save") == 0) - return ip6tables_save_main(argc, argv); - if (strcmp(progname, "restore") == 0) - return ip6tables_restore_main(argc, argv); - - fprintf(stderr, "ip6tables multi-purpose version: " - "unknown subcommand \"%s\"\n", progname); - exit(EXIT_FAILURE); + return subcmd_main(argc, argv, multi6_subcommands); } diff --git a/iptables-multi.c b/iptables-multi.c index 4dcc26d..754b587 100644 --- a/iptables-multi.c +++ b/iptables-multi.c @@ -1,50 +1,26 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <libgen.h> +#include "xshared.h" int iptables_main(int argc, char **argv); int iptables_save_main(int argc, char **argv); int iptables_restore_main(int argc, char **argv); int iptables_xml_main(int argc, char **argv); +static const struct subcommand multi4_subcommands[] = { + {"iptables", iptables_main}, + {"main", iptables_main}, + {"iptables-save", iptables_save_main}, + {"save", iptables_save_main}, + {"iptables-restore", iptables_restore_main}, + {"restore", iptables_restore_main}, + {"iptables-xml", iptables_xml_main}, + {"xml", iptables_xml_main}, + {NULL}, +}; + int main(int argc, char **argv) { - char *progname; - - if (argc < 1) { - fprintf(stderr, "ERROR: This should not happen.\n"); - exit(EXIT_FAILURE); - } - - progname = basename(argv[0]); - if (strcmp(progname, "iptables") == 0) - return iptables_main(argc, argv); - if (strcmp(progname, "iptables-save") == 0) - return iptables_save_main(argc, argv); - if (strcmp(progname, "iptables-restore") == 0) - return iptables_restore_main(argc, argv); - if (strcmp(progname, "iptables-xml") == 0) - return iptables_xml_main(argc, argv); - - ++argv; - --argc; - if (argc < 1) { - fprintf(stderr, "ERROR: No subcommand given.\n"); - exit(EXIT_FAILURE); - } - - progname = basename(argv[0]); - if (strcmp(progname, "main") == 0) - return iptables_main(argc, argv); - if (strcmp(progname, "save") == 0) - return iptables_save_main(argc, argv); - if (strcmp(progname, "restore") == 0) - return iptables_restore_main(argc, argv); - if (strcmp(progname, "xml") == 0) - return iptables_xml_main(argc, argv); - - fprintf(stderr, "iptables multi-purpose version: " - "unknown subcommand \"%s\"\n", progname); - exit(EXIT_FAILURE); + return subcmd_main(argc, argv, multi4_subcommands); } diff --git a/xshared.c b/xshared.c index c5a9015..404a9f5 100644 --- a/xshared.c +++ b/xshared.c @@ -1,7 +1,10 @@ +#include <libgen.h> #include <netdb.h> #include <stdbool.h> #include <stdint.h> #include <stdio.h> +#include <stdlib.h> +#include <string.h> #include <xtables.h> #include "xshared.h" @@ -99,3 +102,36 @@ struct xtables_match *load_proto(struct iptables_command_state *cs) return find_proto(cs->protocol, XTF_TRY_LOAD, cs->options & OPT_NUMERIC, &cs->matches); } + +static mainfunc_t subcmd_get(const char *cmd, const struct subcommand *cb) +{ + for (; cb->name != NULL; ++cb) + if (strcmp(cb->name, cmd) == 0) + return cb->main; + return NULL; +} + +int subcmd_main(int argc, char **argv, const struct subcommand *cb) +{ + const char *cmd = basename(*argv); + mainfunc_t f = subcmd_get(cmd, cb); + + if (f == NULL && argc > 1) { + /* + * Unable to find a main method for our command name? + * Let's try again with the first argument! + */ + ++argv; + --argc; + f = subcmd_get(*argv, cb); + } + + /* now we should have a valid function pointer */ + if (f != NULL) + return f(argc, argv); + + fprintf(stderr, "ERROR: No valid subcommand given.\nValid subcommands:\n"); + for (; cb->name != NULL; ++cb) + fprintf(stderr, " * %s\n", cb->name); + exit(EXIT_FAILURE); +} diff --git a/xshared.h b/xshared.h index a08e6d9..94abb39 100644 --- a/xshared.h +++ b/xshared.h @@ -1,7 +1,10 @@ #ifndef IPTABLES_XSHARED_H #define IPTABLES_XSHARED_H 1 +#include <limits.h> #include <stdint.h> +#include <netinet/in.h> +#include <net/if.h> #include <linux/netfilter_ipv4/ip_tables.h> #include <linux/netfilter_ipv6/ip6_tables.h> @@ -39,6 +42,13 @@ struct iptables_command_state { char **argv; }; +typedef int (*mainfunc_t)(int, char **); + +struct subcommand { + const char *name; + mainfunc_t main; +}; + enum { XT_OPTION_OFFSET_SCALE = 256, }; @@ -47,5 +57,6 @@ extern void print_extension_helps(const struct xtables_target *, const struct xtables_rule_match *); extern const char *proto_to_name(uint8_t, int); extern struct xtables_match *load_proto(struct iptables_command_state *); +extern int subcmd_main(int, char **, const struct subcommand *); #endif /* IPTABLES_XSHARED_H */ -- # Created with git-export-patch -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html