[PATCH nft 0/4] Help and getopt improvements

[PATCH nft 0/4] Help and getopt improvements

[Jeremy Sowden]

I spotted a couple more mistakes in the help.  The first two patches fix
them.  The last patch generates the getopt_long(3) optstring and
options, and the help from one data-structure in a bid to keep them all
in sync.

Jeremy Sowden (4):
  main: include '-d' in help.
  main: include '--reversedns' in help.
  main: interpolate default include path into help format-string.
  main: use one data-structure to initialize getopt_long(3) arguments
    and help.

 src/main.c | 251 +++++++++++++++++++++++++++++------------------------
 1 file changed, 138 insertions(+), 113 deletions(-)

-- 
2.25.1

[PATCH nft 1/4] main: include '-d' in help.

[Jeremy Sowden]

The short option for '--debug' was omitted from the help.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/main.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/src/main.c b/src/main.c
index 81f30951c90f..3e37d600e38b 100644
--- a/src/main.c
+++ b/src/main.c
@@ -140,28 +140,28 @@ static void show_help(const char *name)
 "Usage: %s [ options ] [ cmds... ]\n"
 "\n"
 "Options:\n"
-"  -h, --help			Show this help\n"
-"  -v, --version			Show version information\n"
-"  -V				Show extended version information\n"
+"  -h, --help				Show this help\n"
+"  -v, --version				Show version information\n"
+"  -V					Show extended version information\n"
 "\n"
-"  -c, --check			Check commands validity without actually applying the changes.\n"
-"  -f, --file <filename>		Read input from <filename>\n"
-"  -i, --interactive		Read input from interactive CLI\n"
+"  -c, --check				Check commands validity without actually applying the changes.\n"
+"  -f, --file <filename>			Read input from <filename>\n"
+"  -i, --interactive			Read input from interactive CLI\n"
 "\n"
-"  -j, --json			Format output in JSON\n"
-"  -n, --numeric			Print fully numerical output.\n"
-"  -s, --stateless		Omit stateful information of ruleset.\n"
-"  -t, --terse			Omit contents of sets.\n"
-"  -u, --guid			Print UID/GID as defined in /etc/passwd and /etc/group.\n"
-"  -N				Translate IP addresses to names.\n"
-"  -S, --service			Translate ports to service names as described in /etc/services.\n"
-"  -p, --numeric-protocol	Print layer 4 protocols numerically.\n"
-"  -y, --numeric-priority	Print chain priority numerically.\n"
-"  -T, --numeric-time		Print time values numerically.\n"
-"  -a, --handle			Output rule handle.\n"
-"  -e, --echo			Echo what has been added, inserted or replaced.\n"
-"  -I, --includepath <directory>	Add <directory> to the paths searched for include files. Default is: %s\n"
-"  --debug <level [,level...]>	Specify debugging level (scanner, parser, eval, netlink, mnl, proto-ctx, segtree, all)\n"
+"  -j, --json				Format output in JSON\n"
+"  -n, --numeric				Print fully numerical output.\n"
+"  -s, --stateless			Omit stateful information of ruleset.\n"
+"  -t, --terse				Omit contents of sets.\n"
+"  -u, --guid				Print UID/GID as defined in /etc/passwd and /etc/group.\n"
+"  -N					Translate IP addresses to names.\n"
+"  -S, --service				Translate ports to service names as described in /etc/services.\n"
+"  -p, --numeric-protocol		Print layer 4 protocols numerically.\n"
+"  -y, --numeric-priority		Print chain priority numerically.\n"
+"  -T, --numeric-time			Print time values numerically.\n"
+"  -a, --handle				Output rule handle.\n"
+"  -e, --echo				Echo what has been added, inserted or replaced.\n"
+"  -I, --includepath <directory>		Add <directory> to the paths searched for include files. Default is: %s\n"
+"  -d, --debug <level [,level...]>	Specify debugging level (scanner, parser, eval, netlink, mnl, proto-ctx, segtree, all)\n"
 "\n",
 	name, DEFAULT_INCLUDE_PATH);
 }
-- 
2.25.1

[PATCH nft 2/4] main: include '--reversedns' in help.

[Jeremy Sowden]

The long option for '-N' was omitted from the help.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/main.c b/src/main.c
index 3e37d600e38b..d00c7ec2b687 100644
--- a/src/main.c
+++ b/src/main.c
@@ -153,7 +153,7 @@ static void show_help(const char *name)
 "  -s, --stateless			Omit stateful information of ruleset.\n"
 "  -t, --terse				Omit contents of sets.\n"
 "  -u, --guid				Print UID/GID as defined in /etc/passwd and /etc/group.\n"
-"  -N					Translate IP addresses to names.\n"
+"  -N, --reversedns			Translate IP addresses to names.\n"
 "  -S, --service				Translate ports to service names as described in /etc/services.\n"
 "  -p, --numeric-protocol		Print layer 4 protocols numerically.\n"
 "  -y, --numeric-priority		Print chain priority numerically.\n"
-- 
2.25.1

[PATCH nft 3/4] main: interpolate default include path into help format-string.

[Jeremy Sowden]

The default include path is a string literal defined as a preprocessor
macro by autoconf.  We can just interpolate it.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/main.c b/src/main.c
index d00c7ec2b687..4e9a2ed3bcec 100644
--- a/src/main.c
+++ b/src/main.c
@@ -160,10 +160,10 @@ static void show_help(const char *name)
 "  -T, --numeric-time			Print time values numerically.\n"
 "  -a, --handle				Output rule handle.\n"
 "  -e, --echo				Echo what has been added, inserted or replaced.\n"
-"  -I, --includepath <directory>		Add <directory> to the paths searched for include files. Default is: %s\n"
+"  -I, --includepath <directory>		Add <directory> to the paths searched for include files. Default is: " DEFAULT_INCLUDE_PATH "\n"
 "  -d, --debug <level [,level...]>	Specify debugging level (scanner, parser, eval, netlink, mnl, proto-ctx, segtree, all)\n"
 "\n",
-	name, DEFAULT_INCLUDE_PATH);
+	name);
 }
 
 static void show_version(void)
-- 
2.25.1

[PATCH nft 4/4] main: use one data-structure to initialize getopt_long(3) arguments and help.

[Jeremy Sowden]

By generating the getopt_long(3) optstring and options, and the help
from one source, we reduce the chance that they may get out of sync.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/main.c | 251 +++++++++++++++++++++++++++++------------------------
 1 file changed, 138 insertions(+), 113 deletions(-)

diff --git a/src/main.c b/src/main.c
index 4e9a2ed3bcec..1ee4d957a152 100644
--- a/src/main.c
+++ b/src/main.c
@@ -24,6 +24,19 @@
 
 static struct nft_ctx *nft;
 
+/*
+ * These options are grouped separately in the help, so we give them named
+ * indices for use there.
+ */
+enum opt_indices {
+	IDX_HELP,
+	IDX_VERSION,
+	IDX_VERSION_LONG,
+	IDX_CHECK,
+	IDX_FILE,
+	IDX_INTERACTIVE,
+};
+
 enum opt_vals {
 	OPT_HELP		= 'h',
 	OPT_VERSION		= 'v',
@@ -47,123 +60,133 @@ enum opt_vals {
 	OPT_TERSE		= 't',
 	OPT_INVALID		= '?',
 };
-#define OPTSTRING	"+hvVcf:iI:jnsNSd:aeuypTt"
 
-static const struct option options[] = {
-	{
-		.name		= "help",
-		.val		= OPT_HELP,
-	},
-	{
-		.name		= "version",
-		.val		= OPT_VERSION,
-	},
-	{
-		.name		= "check",
-		.val		= OPT_CHECK,
-	},
-	{
-		.name		= "file",
-		.val		= OPT_FILE,
-		.has_arg	= 1,
-	},
-	{
-		.name		= "interactive",
-		.val		= OPT_INTERACTIVE,
-	},
-	{
-		.name		= "numeric",
-		.val		= OPT_NUMERIC,
-	},
-	{
-		.name		= "stateless",
-		.val		= OPT_STATELESS,
-	},
-	{
-		.name		= "reversedns",
-		.val		= OPT_IP2NAME,
-	},
-	{
-		.name		= "service",
-		.val		= OPT_SERVICE,
-	},
-	{
-		.name		= "includepath",
-		.val		= OPT_INCLUDEPATH,
-		.has_arg	= 1,
-	},
-	{
-		.name		= "debug",
-		.val		= OPT_DEBUG,
-		.has_arg	= 1,
-	},
-	{
-		.name		= "handle",
-		.val		= OPT_HANDLE_OUTPUT,
-	},
-	{
-		.name		= "echo",
-		.val		= OPT_ECHO,
-	},
-	{
-		.name		= "json",
-		.val		= OPT_JSON,
-	},
-	{
-		.name		= "guid",
-		.val		= OPT_GUID,
-	},
-	{
-		.name		= "numeric-priority",
-		.val		= OPT_NUMERIC_PRIO,
-	},
-	{
-		.name		= "numeric-protocol",
-		.val		= OPT_NUMERIC_PROTO,
-	},
-	{
-		.name		= "numeric-time",
-		.val		= OPT_NUMERIC_TIME,
-	},
-	{
-		.name		= "terse",
-		.val		= OPT_TERSE,
-	},
-	{
-		.name		= NULL
-	}
+struct nft_opt {
+	const char    *name;
+	enum opt_vals  val;
+	const char    *arg;
+	const char    *help;
+};
+
+#define NFT_OPT(n, v, a, h) \
+	(struct nft_opt) { .name = n, .val = v, .arg = a, .help = h }
+
+static const struct nft_opt nft_options[] = {
+	NFT_OPT("help",			OPT_HELP,		NULL,
+		"Show this help"),
+	NFT_OPT("version",		OPT_VERSION,		NULL,
+		"Show version information"),
+	NFT_OPT(NULL,                   OPT_VERSION_LONG,       NULL,
+		"Show extended version information"),
+	NFT_OPT("check",		OPT_CHECK,		NULL,
+		"Check commands validity without actually applying the changes."),
+	NFT_OPT("file",			OPT_FILE,		"<filename>",
+		"Read input from <filename>"),
+	NFT_OPT("interactive",		OPT_INTERACTIVE,	NULL,
+		"Read input from interactive CLI"),
+	NFT_OPT("numeric",		OPT_NUMERIC,		NULL,
+		"Print fully numerical output."),
+	NFT_OPT("stateless",		OPT_STATELESS,		NULL,
+		"Omit stateful information of ruleset."),
+	NFT_OPT("reversedns",		OPT_IP2NAME,		NULL,
+		"Translate IP addresses to names."),
+	NFT_OPT("service",		OPT_SERVICE,		NULL,
+		"Translate ports to service names as described in /etc/services."),
+	NFT_OPT("includepath",		OPT_INCLUDEPATH,	"<directory>",
+		"Add <directory> to the paths searched for include files. Default is: " DEFAULT_INCLUDE_PATH),
+	NFT_OPT("debug",		OPT_DEBUG,		"<level [,level...]>",
+		"Specify debugging level (scanner, parser, eval, netlink, mnl, proto-ctx, segtree, all)"),
+	NFT_OPT("handle",		OPT_HANDLE_OUTPUT,	NULL,
+		"Output rule handle."),
+	NFT_OPT("echo",			OPT_ECHO,		NULL,
+		"Echo what has been added, inserted or replaced."),
+	NFT_OPT("json",			OPT_JSON,		NULL,
+		"Format output in JSON"),
+	NFT_OPT("guid",			OPT_GUID,		NULL,
+		"Print UID/GID as defined in /etc/passwd and /etc/group."),
+	NFT_OPT("numeric-priority",	OPT_NUMERIC_PRIO,	NULL,
+		"Print chain priority numerically."),
+	NFT_OPT("numeric-protocol",	OPT_NUMERIC_PROTO,	NULL,
+		"Print layer 4 protocols numerically."),
+	NFT_OPT("numeric-time",		OPT_NUMERIC_TIME,	NULL,
+		"Print time values numerically."),
+	NFT_OPT("terse",		OPT_TERSE,		NULL,
+		"Omit contents of sets."),
 };
 
+#define NR_NFT_OPTIONS (sizeof(nft_options) / sizeof(nft_options[0]))
+
+static const char *get_optstring(void)
+{
+	static char optstring[NR_NFT_OPTIONS + 2];
+
+	if (!optstring[0]) {
+		size_t i, j;
+
+		optstring[0] = '+';
+		for (i = 0, j = 1; i < NR_NFT_OPTIONS; i++)
+			j += sprintf(optstring + j, "%c%s",
+				     nft_options[i].val,
+				     nft_options[i].arg ? ":" : "");
+	}
+	return optstring;
+}
+
+static const struct option *get_options(void)
+{
+	static struct option options[NR_NFT_OPTIONS + 1];
+
+	if (!options[0].name) {
+		size_t i, j;
+
+		for (i = 0, j = 0; i < NR_NFT_OPTIONS; ++i) {
+			if (nft_options[i].name) {
+				options[j].name    = nft_options[i].name;
+				options[j].val     = nft_options[i].val;
+				options[j].has_arg = nft_options[i].arg != NULL;
+				j++;
+			}
+		}
+	}
+	return options;
+}
+
+static void print_option(const struct nft_opt *opt)
+{
+	char optbuf[33] = "";
+	int i;
+
+	i = snprintf(optbuf, sizeof(optbuf), "  -%c", opt->val);
+	if (opt->name)
+		i += snprintf(optbuf + i, sizeof(optbuf) - i, ", %s", opt->name);
+	if (opt->arg)
+		i += snprintf(optbuf + i, sizeof(optbuf) - i, " %s", opt->arg);
+
+	printf("%-32s%s\n", optbuf, opt->help);
+}
+
 static void show_help(const char *name)
 {
-	printf(
-"Usage: %s [ options ] [ cmds... ]\n"
-"\n"
-"Options:\n"
-"  -h, --help				Show this help\n"
-"  -v, --version				Show version information\n"
-"  -V					Show extended version information\n"
-"\n"
-"  -c, --check				Check commands validity without actually applying the changes.\n"
-"  -f, --file <filename>			Read input from <filename>\n"
-"  -i, --interactive			Read input from interactive CLI\n"
-"\n"
-"  -j, --json				Format output in JSON\n"
-"  -n, --numeric				Print fully numerical output.\n"
-"  -s, --stateless			Omit stateful information of ruleset.\n"
-"  -t, --terse				Omit contents of sets.\n"
-"  -u, --guid				Print UID/GID as defined in /etc/passwd and /etc/group.\n"
-"  -N, --reversedns			Translate IP addresses to names.\n"
-"  -S, --service				Translate ports to service names as described in /etc/services.\n"
-"  -p, --numeric-protocol		Print layer 4 protocols numerically.\n"
-"  -y, --numeric-priority		Print chain priority numerically.\n"
-"  -T, --numeric-time			Print time values numerically.\n"
-"  -a, --handle				Output rule handle.\n"
-"  -e, --echo				Echo what has been added, inserted or replaced.\n"
-"  -I, --includepath <directory>		Add <directory> to the paths searched for include files. Default is: " DEFAULT_INCLUDE_PATH "\n"
-"  -d, --debug <level [,level...]>	Specify debugging level (scanner, parser, eval, netlink, mnl, proto-ctx, segtree, all)\n"
-"\n",
-	name);
+	printf("Usage: %s [ options ] [ cmds... ]\n"
+	       "\n"
+	       "Options:\n", name);
+
+	print_option(&nft_options[IDX_HELP]);
+	print_option(&nft_options[IDX_VERSION]);
+	print_option(&nft_options[IDX_VERSION_LONG]);
+
+	fputs("\n", stdout);
+
+	print_option(&nft_options[IDX_CHECK]);
+	print_option(&nft_options[IDX_FILE]);
+	print_option(&nft_options[IDX_INTERACTIVE]);
+
+	fputs("\n", stdout);
+
+	for (size_t i = IDX_INTERACTIVE + 1; i < NR_NFT_OPTIONS; ++i)
+		print_option(&nft_options[i]);
+
+	fputs("\n", stdout);
 }
 
 static void show_version(void)
@@ -288,6 +311,8 @@ static bool nft_options_check(int argc, char * const argv[])
 
 int main(int argc, char * const *argv)
 {
+	const struct option *options = get_options();
+	const char *optstring = get_optstring();
 	char *buf = NULL, *filename = NULL;
 	unsigned int output_flags = 0;
 	bool interactive = false;
@@ -301,7 +326,7 @@ int main(int argc, char * const *argv)
 	nft = nft_ctx_new(NFT_CTX_DEFAULT);
 
 	while (1) {
-		val = getopt_long(argc, argv, OPTSTRING, options, NULL);
+		val = getopt_long(argc, argv, optstring, options, NULL);
 		if (val == -1)
 			break;
 
-- 
2.25.1

Re: [PATCH nft 0/4] Help and getopt improvements

[Pablo Neira Ayuso]

On Thu, Mar 05, 2020 at 02:48:01PM +0000, Jeremy Sowden wrote:
> I spotted a couple more mistakes in the help.  The first two patches fix
> them.  The last patch generates the getopt_long(3) optstring and
> options, and the help from one data-structure in a bid to keep them all
> in sync.

Series applied, thanks.