From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Michael Zintakis <michael.zintakis@googlemail.com>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [PATCH v3 nfacct 7/29] code-refactoring changes to the "command menu"
Date: Tue, 16 Jul 2013 00:41:13 +0200 [thread overview]
Message-ID: <20130715224113.GC5405@localhost> (raw)
In-Reply-To: <1373480727-11254-8-git-send-email-michael.zintakis@googlemail.com>
On Wed, Jul 10, 2013 at 07:25:05PM +0100, Michael Zintakis wrote:
> * replace the existing "command menu" with more efficient code, which is
> clearer and easier to maintain;
Can you provide some numbers to prove that efficiency gain?
> * prevent wrong number of command-line arguments being specified
>
> * prevent the correct command line parameters being specified more than once
> (like "nfacct list xml xml" for example)
>
> Signed-off-by: Michael Zintakis <michael.zintakis@googlemail.com>
> ---
> src/nfacct.c | 253 ++++++++++++++++++++++++++++-------------------------------
> 1 file changed, 119 insertions(+), 134 deletions(-)
>
> diff --git a/src/nfacct.c b/src/nfacct.c
> index bf50f50..fbf9aa6 100644
> --- a/src/nfacct.c
> +++ b/src/nfacct.c
> @@ -23,18 +23,6 @@
> #include <libmnl/libmnl.h>
> #include <libnetfilter_acct/libnetfilter_acct.h>
>
> -enum {
> - NFACCT_CMD_NONE = 0,
> - NFACCT_CMD_LIST,
> - NFACCT_CMD_ADD,
> - NFACCT_CMD_DELETE,
> - NFACCT_CMD_GET,
> - NFACCT_CMD_FLUSH,
> - NFACCT_CMD_VERSION,
> - NFACCT_CMD_HELP,
> - NFACCT_CMD_RESTORE,
> -};
> -
> static int nfacct_cmd_list(int argc, char *argv[]);
> static int nfacct_cmd_add(int argc, char *argv[]);
> static int nfacct_cmd_delete(int argc, char *argv[]);
> @@ -44,9 +32,57 @@ static int nfacct_cmd_version(int argc, char *argv[]);
> static int nfacct_cmd_help(int argc, char *argv[]);
> static int nfacct_cmd_restore(int argc, char *argv[]);
>
> +/* Matches two strings, including partial matches */
> +static int nfacct_matches(const char *cmd, const char *pattern)
> +{
> + size_t len;
> +
> + if (cmd == NULL || pattern == NULL)
> + return 0;
> +
> + len = strlen(cmd);
> + if (len == 0 || len > strlen(pattern))
> + return 0;
These checking above are superfluous.
> + return (strncmp(cmd, pattern, len) == 0);
This shortened matching is not described in the patch description and
it does not handle clashes.
> +}
> +
> +/* main command 'menu' */
> +static const struct cmd {
> + const char *cmd;
> + int (*func)(int argc, char **argv);
> +} cmds[] = {
> + { "list", nfacct_cmd_list },
> + { "add", nfacct_cmd_add },
> + { "delete", nfacct_cmd_delete },
> + { "get", nfacct_cmd_get },
> + { "flush", nfacct_cmd_flush },
> + { "restore", nfacct_cmd_restore },
> + { "version", nfacct_cmd_version },
> + { "help", nfacct_cmd_help },
> + { NULL, NULL }
> +};
> +
> +static int nfacct_do_cmd(const char *argv0, int argc, char **argv)
> +{
> + const struct cmd *c;
> +
> + for (c = cmds; c->cmd; ++c) {
> + if (nfacct_matches(argv0, c->cmd)) {
> + return (c->func(argc-1, argv+1));
> + }
> + }
> + fprintf(stderr, "nfacct v%s: Unknown command: %s\n",
> + VERSION, argv0);
> + return EXIT_FAILURE;
> +}
> +
> +
> +static void usage(char *argv[]) __attribute__((noreturn));
> static void usage(char *argv[])
> {
> fprintf(stderr, "Usage: %s command [parameters]...\n", argv[0]);
> + exit(EXIT_FAILURE);
> }
>
> static void nfacct_perror(const char *msg)
> @@ -59,80 +95,24 @@ static void nfacct_perror(const char *msg)
> }
> }
>
> -/* Matches two strings, including partial matches */
> -static int nfacct_matches(const char *cmd, const char *pattern)
> -{
> - size_t len;
> -
> - if (cmd == NULL || pattern == NULL)
> - return 0;
> -
> - len = strlen(cmd);
> - if (len == 0 || len > strlen(pattern))
> - return 0;
> -
> - return (strncmp(cmd, pattern, len) == 0);
> -}
> +#define NFACCT_RET_ERR(x) nfacct_perror(x); \
> + goto err;
Hiding a goto in a macro is not good idea.
> +#define NFACCT_RET_ARG_ERR() NFACCT_RET_ERR("unknown argument")
>
> +#define NFACCT_NEXT_ARG() do { \
> + argv++; \
> + argc--; \
> + } while(0)
>
> int main(int argc, char *argv[])
> {
> - int cmd = NFACCT_CMD_NONE, ret = 0;
> -
> - if (argc < 2) {
> - usage(argv);
> - exit(EXIT_FAILURE);
> - }
> -
> - if (nfacct_matches(argv[1], "list"))
> - cmd = NFACCT_CMD_LIST;
> - else if (nfacct_matches(argv[1], "add"))
> - cmd = NFACCT_CMD_ADD;
> - else if (nfacct_matches(argv[1], "delete"))
> - cmd = NFACCT_CMD_DELETE;
> - else if (nfacct_matches(argv[1], "get"))
> - cmd = NFACCT_CMD_GET;
> - else if (nfacct_matches(argv[1], "flush"))
> - cmd = NFACCT_CMD_FLUSH;
> - else if (nfacct_matches(argv[1], "version"))
> - cmd = NFACCT_CMD_VERSION;
> - else if (nfacct_matches(argv[1], "help"))
> - cmd = NFACCT_CMD_HELP;
> - else if (nfacct_matches(argv[1], "restore"))
> - cmd = NFACCT_CMD_RESTORE;
> - else {
> - fprintf(stderr, "nfacct v%s: Unknown command: %s\n",
> - VERSION, argv[1]);
> - usage(argv);
> - exit(EXIT_FAILURE);
> - }
> -
> - switch(cmd) {
> - case NFACCT_CMD_LIST:
> - ret = nfacct_cmd_list(argc, argv);
> - break;
> - case NFACCT_CMD_ADD:
> - ret = nfacct_cmd_add(argc, argv);
> - break;
> - case NFACCT_CMD_DELETE:
> - ret = nfacct_cmd_delete(argc, argv);
> - break;
> - case NFACCT_CMD_GET:
> - ret = nfacct_cmd_get(argc, argv);
> - break;
> - case NFACCT_CMD_FLUSH:
> - ret = nfacct_cmd_flush(argc, argv);
> - break;
> - case NFACCT_CMD_VERSION:
> - ret = nfacct_cmd_version(argc, argv);
> - break;
> - case NFACCT_CMD_HELP:
> - ret = nfacct_cmd_help(argc, argv);
> - break;
> - case NFACCT_CMD_RESTORE:
> - ret = nfacct_cmd_restore(argc, argv);
> - break;
> - }
> - return ret < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
> + int ret = 0;
> +
> + if (argc >= 2) {
> + ret = nfacct_do_cmd(argv[1], argc-1, argv+1);
> + if (ret != EXIT_FAILURE)
> + return ret < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
> + }
> + usage(argv);
> }
>
> static bool xml_header = false;
> @@ -174,29 +154,29 @@ err:
>
> static int nfacct_cmd_list(int argc, char *argv[])
> {
> - bool zeroctr = false, xml = false;
> + bool nfnl_msg = false, xml = false;
> struct mnl_socket *nl;
> char buf[MNL_SOCKET_BUFFER_SIZE];
> struct nlmsghdr *nlh;
> unsigned int seq, portid;
> - int ret, i;
> + int ret;
> + uint8_t nfnl_cmd = NFNL_MSG_ACCT_GET;
>
> - for (i=2; i<argc; i++) {
> - if (nfacct_matches(argv[i], "reset")) {
> - zeroctr = true;
> - } else if (nfacct_matches(argv[i], "xml")) {
> + while (argc > 0) {
> + if (!nfnl_msg && nfacct_matches(argv[0],"reset")) {
^
space after comma
> + nfnl_cmd = NFNL_MSG_ACCT_GET_CTRZERO;
> + nfnl_msg = true;
> + } else if (!xml && nfacct_matches(argv[0],"xml")) {
> xml = true;
> } else {
> nfacct_perror("unknown argument");
> return -1;
> }
> + argc--; argv++;
> }
>
> seq = time(NULL);
> - nlh = nfacct_nlmsg_build_hdr(buf, zeroctr ?
> - NFNL_MSG_ACCT_GET_CTRZERO :
> - NFNL_MSG_ACCT_GET,
> - NLM_F_DUMP, seq);
> + nlh = nfacct_nlmsg_build_hdr(buf, nfnl_cmd, NLM_F_DUMP, seq);
>
> nl = mnl_socket_open(NETLINK_NETFILTER);
> if (nl == NULL) {
> @@ -298,15 +278,15 @@ static int _nfacct_cmd_add(char *name, uint64_t pkts, uint64_t bytes)
>
> static int nfacct_cmd_add(int argc, char *argv[])
> {
> - if (argc < 3 || strlen(argv[2]) == 0) {
> + if (argc < 1 || strlen(argv[0]) == 0) {
> nfacct_perror("missing object name");
> return -1;
> - } else if (argc > 3) {
> + } else if (argc > 1) {
> nfacct_perror("too many arguments");
> return -1;
> }
>
> - return _nfacct_cmd_add(argv[2], 0, 0);
> + return _nfacct_cmd_add(argv[0], 0, 0);
> }
>
> static int nfacct_cmd_delete(int argc, char *argv[])
> @@ -318,10 +298,10 @@ static int nfacct_cmd_delete(int argc, char *argv[])
> struct nfacct *nfacct;
> int ret;
>
> - if (argc < 3 || strlen(argv[2]) == 0) {
> + if (argc < 1 || strlen(argv[0]) == 0) {
> nfacct_perror("missing object name");
> return -1;
> - } else if (argc > 3) {
> + } else if (argc > 1) {
> nfacct_perror("too many arguments");
> return -1;
> }
> @@ -332,7 +312,7 @@ static int nfacct_cmd_delete(int argc, char *argv[])
> return -1;
> }
>
> - nfacct_attr_set(nfacct, NFACCT_ATTR_NAME, argv[2]);
> + nfacct_attr_set(nfacct, NFACCT_ATTR_NAME, argv[0]);
>
> seq = time(NULL);
> nlh = nfacct_nlmsg_build_hdr(buf, NFNL_MSG_ACCT_DEL,
> @@ -377,41 +357,46 @@ static int nfacct_cmd_delete(int argc, char *argv[])
>
> static int nfacct_cmd_get(int argc, char *argv[])
> {
> - bool zeroctr = false, xml = false;
> + bool nfnl_msg = false, xml = false;
> struct mnl_socket *nl;
> char buf[MNL_SOCKET_BUFFER_SIZE];
> struct nlmsghdr *nlh;
> uint32_t portid, seq;
> struct nfacct *nfacct;
> - int ret, i;
> + int ret = -1, i;
> + char *name;
> + uint8_t nfnl_cmd = NFNL_MSG_ACCT_GET;
>
> - if (argc < 3 || strlen(argv[2]) == 0) {
> + if (argc < 1 || strlen(argv[0]) == 0) {
> nfacct_perror("missing object name");
> return -1;
> }
> - for (i=3; i<argc; i++) {
> - if (nfacct_matches(argv[i], "reset")) {
> - zeroctr = true;
> - } else if (nfacct_matches(argv[i], "xml")) {
> + name = strdup(argv[0]);
> + if (!name) {
> + nfacct_perror("OOM");
> + return -1;
> + }
> + NFACCT_NEXT_ARG();
> + while (argc > 0) {
> + if (!nfnl_msg && nfacct_matches(argv[0],"reset")) {
> + nfnl_cmd = NFNL_MSG_ACCT_GET_CTRZERO;
> + nfnl_msg = true;
> + } else if (!xml && nfacct_matches(argv[0],"xml")) {
> xml = true;
> } else {
> - nfacct_perror("unknown argument");
> - return -1;
> + NFACCT_RET_ARG_ERR();
> }
> + argc--; argv++;
> }
>
> nfacct = nfacct_alloc();
> if (nfacct == NULL) {
> - nfacct_perror("OOM");
> - return -1;
> + NFACCT_RET_ERR("OOM");
> }
> - nfacct_attr_set(nfacct, NFACCT_ATTR_NAME, argv[2]);
> + nfacct_attr_set(nfacct, NFACCT_ATTR_NAME, name);
>
> seq = time(NULL);
> - nlh = nfacct_nlmsg_build_hdr(buf, zeroctr ?
> - NFNL_MSG_ACCT_GET_CTRZERO :
> - NFNL_MSG_ACCT_GET,
> - NLM_F_ACK, seq);
> + nlh = nfacct_nlmsg_build_hdr(buf, nfnl_cmd, NLM_F_ACK, seq);
>
> nfacct_nlmsg_build_payload(nlh, nfacct);
>
> @@ -419,38 +404,38 @@ static int nfacct_cmd_get(int argc, char *argv[])
>
> nl = mnl_socket_open(NETLINK_NETFILTER);
> if (nl == NULL) {
> - nfacct_perror("mnl_socket_open");
> - return -1;
> + NFACCT_RET_ERR("mnl_socket_open");
> }
>
> if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0) {
> - nfacct_perror("mnl_socket_bind");
> - return -1;
> + NFACCT_RET_ERR("mnl_socket_bind");
> }
> portid = mnl_socket_get_portid(nl);
>
> if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
> - nfacct_perror("mnl_socket_send");
> - return -1;
> + NFACCT_RET_ERR("mnl_socket_send");
> }
>
> - ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
> - while (ret > 0) {
> - ret = mnl_cb_run(buf, ret, seq, portid, nfacct_cb, &xml);
> - if (ret <= 0)
> + i = mnl_socket_recvfrom(nl, buf, sizeof(buf));
> + while (i > 0) {
> + i = mnl_cb_run(buf, i, seq, portid, nfacct_cb, &xml);
> + if (i <= 0)
> break;
> - ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
> + i = mnl_socket_recvfrom(nl, buf, sizeof(buf));
> }
> - if (ret == -1) {
> - nfacct_perror("error");
> - return -1;
> + if (i == -1) {
> + NFACCT_RET_ERR("error");
> }
> mnl_socket_close(nl);
>
> if (xml_header)
> printf("</nfacct>\n");
>
> - return 0;
> + ret = 0;
> +
> +err:
> + free(name);
> + return ret;
> }
>
> static int nfacct_cmd_flush(int argc, char *argv[])
> @@ -461,7 +446,7 @@ static int nfacct_cmd_flush(int argc, char *argv[])
> uint32_t portid, seq;
> int ret;
>
> - if (argc > 2) {
> + if (argc > 0) {
> nfacct_perror("too many arguments");
> return -1;
> }
> @@ -522,7 +507,7 @@ static int nfacct_cmd_version(int argc, char *argv[])
> static const char help_msg[] =
> "nfacct v%s: utility for the Netfilter extended accounting "
> "infrastructure\n"
> - "Usage: %s command [parameters]...\n\n"
> + "Usage: nfacct command [parameters]...\n\n"
> "Commands:\n"
> " list [reset]\t\tList the accounting object table (and reset)\n"
> " add object-name\tAdd new accounting object to table\n"
> @@ -535,7 +520,7 @@ static const char help_msg[] =
>
> static int nfacct_cmd_help(int argc, char *argv[])
> {
> - printf(help_msg, VERSION, argv[0]);
> + printf(help_msg, VERSION);
> return 0;
> }
>
> @@ -546,7 +531,7 @@ static int nfacct_cmd_restore(int argc, char *argv[])
> char buffer[512];
> int ret;
>
> - if (argc > 2) {
> + if (argc > 0) {
> nfacct_perror("too many arguments");
> return -1;
> }
> --
> 1.8.3.1
>
next prev parent reply other threads:[~2013-07-15 22:41 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-10 18:24 [PATCH v3 0/29] nfacct changes and additions Michael Zintakis
2013-07-10 18:24 ` [PATCH v3 kernel 1/29] bugfix: pkts/bytes need to be specified simultaneously Michael Zintakis
2013-07-10 20:04 ` Florian Westphal
2013-07-11 18:56 ` Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 kernel 2/29] bugfix: restore pkts/bytes counters in NLM_F_REPLACE Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 3/29] bugfix: correct xml name parsing Michael Zintakis
2013-07-15 22:24 ` Pablo Neira Ayuso
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 4/29] bugfix: correct (plain) " Michael Zintakis
2013-07-15 22:29 ` Pablo Neira Ayuso
2013-07-10 18:25 ` [PATCH v3 nfacct 5/29] bugfix: prevent 0-sized parameter being accepted Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 6/29] bugfix: prevent 0-sized nfacct name " Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 7/29] code-refactoring changes to the "command menu" Michael Zintakis
2013-07-15 22:41 ` Pablo Neira Ayuso [this message]
2013-07-10 18:25 ` [PATCH v3 nfacct 8/29] add 2 new options: "replace" and "flush" Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 9/29] add *_SAVE template allowing save/restore Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 10/29] add *_BONLY template to show bytes-only Michael Zintakis
2013-07-15 22:42 ` Pablo Neira Ayuso
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 11/29] add variable width and on-the-fly formatting Michael Zintakis
2013-07-15 22:51 ` Pablo Neira Ayuso
2013-07-10 18:25 ` [PATCH v3 nfacct 12/29] add variable width and on-the-fly number formatting Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 13/29] add new "save" and correct existing "restore" commands Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 14/29] add sort option to the "list" command Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 15/29] add "show bytes" option to "list" and "get" commands Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 kernel 16/29] add permanent byte/packet format capability to nfacct Michael Zintakis
2013-07-10 20:00 ` Florian Westphal
2013-07-11 18:56 ` Michael Zintakis
2013-07-11 20:12 ` Florian Westphal
2013-07-14 8:29 ` Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 17/29] add *permanent* number formatting support Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 18/29] add permanent number formatting to nfacct objects Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 kernel 19/29] add byte threshold capability to nfacct Michael Zintakis
2013-07-10 20:00 ` Florian Westphal
2013-07-11 18:56 ` Michael Zintakis
2013-07-11 20:25 ` Florian Westphal
2013-07-17 19:44 ` Alexey Perevalov
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 20/29] add byte threshold capability support Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 21/29] add byte threshold capabilities to nfacct objects Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 22/29] add *_EXTENDED template support Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 23/29] add "show extended" option to "list" and "get" commands Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 kernel 24/29] add packets and bytes mark capability to nfacct Michael Zintakis
2013-07-10 20:01 ` Florian Westphal
2013-07-11 18:56 ` Michael Zintakis
2013-07-11 1:14 ` Pablo Neira Ayuso
2013-07-11 18:56 ` Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 25/29] add packets/bytes mark capability support Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 26/29] add setmark and clrmark to "get" and "list" commands Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 libnetfilter_acct 27/29] add *_MONLY template support Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 28/29] add "show marks" option to "list" and "get" commands Michael Zintakis
2013-07-10 18:25 ` [PATCH v3 nfacct 29/29] change man page to describe all new features Michael Zintakis
2013-07-15 12:36 ` [0/29] nfacct changes and additions Pablo Neira Ayuso
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130715224113.GC5405@localhost \
--to=pablo@netfilter.org \
--cc=michael.zintakis@googlemail.com \
--cc=netfilter-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).