| Message ID | 1410450640-2414-2-git-send-email-a.perevalov@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
On Thu, Sep 11, 2014 at 07:50:39PM +0400, Alexey Perevalov wrote: > It was possible to specify several equal options for list operation. > > Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> > --- > src/nfacct.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/src/nfacct.c b/src/nfacct.c > index 091a5c9..d77a57e 100644 > --- a/src/nfacct.c > +++ b/src/nfacct.c > @@ -25,6 +25,13 @@ > #include <linux/netfilter/nfnetlink_acct.h> > #include <linux/netfilter/nfnetlink.h> > > +#define CHECK_OPT_PLURARITY(opt_name, msg) \ > + opt_name += 1; \ > + if (opt_name > 1) { \ > + nfacct_perror(msg); \ > + return -1; \ > + } > + > enum { > NFACCT_CMD_NONE = 0, > NFACCT_CMD_LIST, > @@ -166,6 +173,8 @@ err: > return MNL_CB_OK; > } > > +#define NFACCT_F_QUOTAS (NFACCT_F_QUOTA_BYTES | NFACCT_F_QUOTA_PKTS) > + > static int nfacct_cmd_list(int argc, char *argv[]) > { > bool zeroctr = false, xml = false; > @@ -177,8 +186,14 @@ static int nfacct_cmd_list(int argc, char *argv[]) > > for (i=2; i<argc; i++) { > if (strncmp(argv[i], "reset", strlen(argv[i])) == 0) { > + static int opt_reset; > + CHECK_OPT_PLURARITY(opt_reset, "reset couldn't be " > + "defined more than once"); I prefer if you add something similar to what iproute2 provides: void duparg2(const char *key, const char *arg) { fprintf(stderr, "Error: either \"%s\" is duplicate, or \"%s\" is a garbage.\n", key, arg); exit(-1); } You can probably check: if (zeroctr) duparg2("reset", argv[i]); > zeroctr = true; > } else if (strncmp(argv[i], "xml", strlen(argv[i])) == 0) { > + static int opt_xml; > + CHECK_OPT_PLURARITY(opt_xml, "xml couldn't be defined " > + "more than once"); Similar thing here. I always wanted to get this code more in line iproute2, but failed to find the time, later. So please, do it the way I'm proposing so we start looking into converging to iproute2. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2014 08:55 PM, Pablo Neira Ayuso wrote: > On Thu, Sep 11, 2014 at 07:50:39PM +0400, Alexey Perevalov wrote: >> It was possible to specify several equal options for list operation. >> >> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> >> --- >> src/nfacct.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/src/nfacct.c b/src/nfacct.c >> index 091a5c9..d77a57e 100644 >> --- a/src/nfacct.c >> +++ b/src/nfacct.c >> @@ -25,6 +25,13 @@ >> #include <linux/netfilter/nfnetlink_acct.h> >> #include <linux/netfilter/nfnetlink.h> >> >> +#define CHECK_OPT_PLURARITY(opt_name, msg) \ >> + opt_name += 1; \ >> + if (opt_name > 1) { \ >> + nfacct_perror(msg); \ >> + return -1; \ >> + } >> + >> enum { >> NFACCT_CMD_NONE = 0, >> NFACCT_CMD_LIST, >> @@ -166,6 +173,8 @@ err: >> return MNL_CB_OK; >> } >> >> +#define NFACCT_F_QUOTAS (NFACCT_F_QUOTA_BYTES | NFACCT_F_QUOTA_PKTS) >> + >> static int nfacct_cmd_list(int argc, char *argv[]) >> { >> bool zeroctr = false, xml = false; >> @@ -177,8 +186,14 @@ static int nfacct_cmd_list(int argc, char *argv[]) >> >> for (i=2; i<argc; i++) { >> if (strncmp(argv[i], "reset", strlen(argv[i])) == 0) { >> + static int opt_reset; >> + CHECK_OPT_PLURARITY(opt_reset, "reset couldn't be " >> + "defined more than once"); > I prefer if you add something similar to what iproute2 provides: > > void duparg2(const char *key, const char *arg) > { > fprintf(stderr, "Error: either \"%s\" is duplicate, or \"%s\" is a garbage.\n", key, arg); > exit(-1); > } > > You can probably check: > > if (zeroctr) > duparg2("reset", argv[i]); > >> zeroctr = true; >> } else if (strncmp(argv[i], "xml", strlen(argv[i])) == 0) { >> + static int opt_xml; >> + CHECK_OPT_PLURARITY(opt_xml, "xml couldn't be defined " >> + "more than once"); > Similar thing here. > > I always wanted to get this code more in line iproute2, but failed to > find the time, later. > > So please, do it the way I'm proposing so we start looking into > converging to iproute2. ok, but now nfacct doesn't use iprout2, so suggested function should by copy-pasted, maybe with modification, because "is a garbage" I think no necessary. Do you want to make nfacct code more similar with iproute2 code, to merge it in future? > > Thanks. >
On Fri, Sep 12, 2014 at 12:24:51PM +0400, Alexey Perevalov wrote: > On 09/11/2014 08:55 PM, Pablo Neira Ayuso wrote: > >On Thu, Sep 11, 2014 at 07:50:39PM +0400, Alexey Perevalov wrote: > >>It was possible to specify several equal options for list operation. > >> > >>Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> > >>--- > >> src/nfacct.c | 16 +++++++++++++++- > >> 1 file changed, 15 insertions(+), 1 deletion(-) > >> > >>diff --git a/src/nfacct.c b/src/nfacct.c > >>index 091a5c9..d77a57e 100644 > >>--- a/src/nfacct.c > >>+++ b/src/nfacct.c > >>@@ -25,6 +25,13 @@ > >> #include <linux/netfilter/nfnetlink_acct.h> > >> #include <linux/netfilter/nfnetlink.h> > >>+#define CHECK_OPT_PLURARITY(opt_name, msg) \ > >>+ opt_name += 1; \ > >>+ if (opt_name > 1) { \ > >>+ nfacct_perror(msg); \ > >>+ return -1; \ > >>+ } > >>+ > >> enum { > >> NFACCT_CMD_NONE = 0, > >> NFACCT_CMD_LIST, > >>@@ -166,6 +173,8 @@ err: > >> return MNL_CB_OK; > >> } > >>+#define NFACCT_F_QUOTAS (NFACCT_F_QUOTA_BYTES | NFACCT_F_QUOTA_PKTS) > >>+ > >> static int nfacct_cmd_list(int argc, char *argv[]) > >> { > >> bool zeroctr = false, xml = false; > >>@@ -177,8 +186,14 @@ static int nfacct_cmd_list(int argc, char *argv[]) > >> for (i=2; i<argc; i++) { > >> if (strncmp(argv[i], "reset", strlen(argv[i])) == 0) { > >>+ static int opt_reset; > >>+ CHECK_OPT_PLURARITY(opt_reset, "reset couldn't be " > >>+ "defined more than once"); > >I prefer if you add something similar to what iproute2 provides: > > > >void duparg2(const char *key, const char *arg) > >{ > > fprintf(stderr, "Error: either \"%s\" is duplicate, or \"%s\" is a garbage.\n", key, arg); > > exit(-1); > >} > > > >You can probably check: > > > > if (zeroctr) > > duparg2("reset", argv[i]); > > > >> zeroctr = true; > >> } else if (strncmp(argv[i], "xml", strlen(argv[i])) == 0) { > >>+ static int opt_xml; > >>+ CHECK_OPT_PLURARITY(opt_xml, "xml couldn't be defined " > >>+ "more than once"); > >Similar thing here. > > > >I always wanted to get this code more in line iproute2, but failed to > >find the time, later. > > > >So please, do it the way I'm proposing so we start looking into > >converging to iproute2. > ok, but now nfacct doesn't use iprout2, so suggested function should > by copy-pasted, maybe with modification, > because "is a garbage" I think no necessary. Right, that was just an example. Actually, duparg (instead of duparg2) should better fit that. > Do you want to make nfacct code more similar with iproute2 code, to > merge it in future? No for merge, I'm just pointing to it as a coding style reference. The parser is very simple, error reporting and so on is rather simple, and it matches with what we have in nfacct. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/nfacct.c b/src/nfacct.c index 091a5c9..d77a57e 100644 --- a/src/nfacct.c +++ b/src/nfacct.c @@ -25,6 +25,13 @@ #include <linux/netfilter/nfnetlink_acct.h> #include <linux/netfilter/nfnetlink.h> +#define CHECK_OPT_PLURARITY(opt_name, msg) \ + opt_name += 1; \ + if (opt_name > 1) { \ + nfacct_perror(msg); \ + return -1; \ + } + enum { NFACCT_CMD_NONE = 0, NFACCT_CMD_LIST, @@ -166,6 +173,8 @@ err: return MNL_CB_OK; } +#define NFACCT_F_QUOTAS (NFACCT_F_QUOTA_BYTES | NFACCT_F_QUOTA_PKTS) + static int nfacct_cmd_list(int argc, char *argv[]) { bool zeroctr = false, xml = false; @@ -177,8 +186,14 @@ static int nfacct_cmd_list(int argc, char *argv[]) for (i=2; i<argc; i++) { if (strncmp(argv[i], "reset", strlen(argv[i])) == 0) { + static int opt_reset; + CHECK_OPT_PLURARITY(opt_reset, "reset couldn't be " + "defined more than once"); zeroctr = true; } else if (strncmp(argv[i], "xml", strlen(argv[i])) == 0) { + static int opt_xml; + CHECK_OPT_PLURARITY(opt_xml, "xml couldn't be defined " + "more than once"); xml = true; } else { nfacct_perror("unknown argument"); @@ -191,7 +206,6 @@ static int nfacct_cmd_list(int argc, char *argv[]) NFNL_MSG_ACCT_GET_CTRZERO : NFNL_MSG_ACCT_GET, NLM_F_DUMP, seq); - nl = mnl_socket_open(NETLINK_NETFILTER); if (nl == NULL) { nfacct_perror("mnl_socket_open");
It was possible to specify several equal options for list operation. Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> --- src/nfacct.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)