Re: [PATCH v3 1/2] nfacct: check cmd line argument for singleness

[Alexey Perevalov <a.perevalov@samsung.com>]

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.
>


-- 
Best regards,
Alexey Perevalov,
Tizen Developer,
phone: +7 (495) 797 25 00 ext 3969
e-mail: a.perevalov@samsung.com <mailto:a.perevalov@samsumng.com>

Mobile group, Samsung R&D Institute Rus
12 Dvintsev street, building 1
127018, Moscow, Russian Federation