Re: [nft PATCH v3 2/2] src: add import command

["Álvaro Neira Ayuso" <alvaroneay@gmail.com>]

El 10/03/15 a las 11:37, Patrick McHardy escribió:
> On 10.03, Alvaro Neira Ayuso wrote:
>> @@ -275,6 +279,13 @@ struct export {
>>   struct export *export_alloc(uint32_t format);
>>   void export_free(struct export *e);
>>
>> +struct import {
>> +	uint32_t	format;
>> +};
>> +
>> +struct import *import_alloc(uint32_t format);
>> +void import_free(struct import *i);
>
> How about a common struct for both commands? "format", "import_export",
> ...
>

Nice idea Patrick. I'm going to do it.

>> +import_cmd		:	import_format
>
> Same here, please change export_format to something common.
>
>> +static int ruleset_parse_setelems(const struct nft_parse_ctx *ctx)
>> +{
>> +	const struct ruleset_parse *rp;
>> +	struct nft_set *set;
>> +	uint32_t cmd;
>> +	int ret = -1;
>> +
>> +	set = nft_ruleset_ctx_get(ctx, NFT_RULESET_CTX_SET);
>> +	rp = nft_ruleset_ctx_get(ctx, NFT_RULESET_CTX_DATA);
>> +
>> +	cmd = nft_ruleset_ctx_get_u32(ctx, NFT_RULESET_CTX_CMD);
>> +	switch (cmd) {
>> +	case NFT_CMD_ADD:
>> +		ret = mnl_nft_setelem_batch_add(set, 0, rp->nl_ctx->seqnum);
>> +		break;
>> +	case NFT_CMD_DELETE:
>> +		ret = mnl_nft_setelem_batch_del(set, 0, rp->nl_ctx->seqnum);
>> +		break;
>> +	default:
>> +		errno = EOPNOTSUPP;
>> +		break;
>
> This would be a BUG, no? Same question for all similar cases.

I don't think so. If the user are in another kernel that one operation 
is not supported, we will say that it's a bug and I think it's better to 
say only that the operation is not supported. Maybe my point of view is 
wrong, I'm opened to change it if you think that it's better to show a 
bug message.

>
>> +static int do_command_import(struct netlink_ctx *ctx, struct cmd *cmd)
>> +{
>> +	int ret;
>> +	struct nft_parse_err *err;
>> +	struct ruleset_parse rp = {
>> +		.nl_ctx = ctx,
>> +		.cmd = cmd
>
> Please align, makes it easier to read.

Catched.

>
>> +	ret = nft_ruleset_parse_file_cb(cmd->import->format, stdin, err, &rp,
>> +					ruleset_parse_cb);
>> +	if (ret < 0)
>> +		nft_parse_perror("unable to import. Parsing failed", err);
>
> I'd suggest a ": parsing failed", makes it clear that its only a single
> error, not two.

Perfect, I'm going to change it too.

Thanks for the review to Pablo and Patrick. I'm going to work in the 
changes.
--
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