Hi Pablo,
>> +static struct nft_rule_expr_list *>> +create_nat_expr_list(const struct nf_nat_range *r)>> +{>> + struct nft_rule_expr_list *expr_list;>> + struct nft_rule_expr *nat_expr;>> + int registers = 1;>> +>> + expr_list = nft_rule_expr_list_alloc();>> + if (expr_list == NULL)>> + return NULL;> Better allocate this list in nft.c and pass it as parameter. All> extensions will require this, and after that change you can return -1> on error / 0 on success.>> Or simply pass the struct nft_rule object? Then, you can skip patch> [libnftables PATCH 6/7]?
Why not, it's a design preference. I liked the idea extension don't mess
up with the rule and only provides its expression list.
it's less code on libnftables on your idea at least.
>> + .x6_options = DNAT_opts,>> + .translate_to_nft = DNAT_to_nft,> nft_to_translate is missing, right? We need it to print the rule that> is expressed in native format.
Read the very first mail of this thread. It's actually an issue here.
I had the idea of doing the reverse function indeed, but there is a problem:
From iptables to nftables it's easy, since we get a target made by the
right extension, so we directly get the right translation function.
That's what I did.
Now on the reverse way, we don't know at all to which extension the
expression list belongs to, so which translation function to call.
Currently the only way I see it is to loop on all extensions until one
returns successfully.
We should take care of the position in the expression list as well, and
here I see we will need some more functions from libnftables.
I will try a PoC
> Probably you can call this xt_to_nft or struct_to_nft? It would be> shorter and won't require realigning dnat_tg_reg I would like to skip> those to avoid possible conflicts when merging this, we have more than> 100 extensions.>> BTW, some short description on the patches is a good idea, a couple of> lines description the intention after this (I know well what you're> making but others may not).
Sure.
Tomasz
--
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

Hi Tomasz,
On Wed, May 15, 2013 at 09:48:28AM +0300, Tomasz Bursztyka wrote:
[...]
> >>+static struct nft_rule_expr_list *> >>+create_nat_expr_list(const struct nf_nat_range *r)> >>+{> >>+ struct nft_rule_expr_list *expr_list;> >>+ struct nft_rule_expr *nat_expr;> >>+ int registers = 1;> >>+> >>+ expr_list = nft_rule_expr_list_alloc();> >>+ if (expr_list == NULL)> >>+ return NULL;> >Better allocate this list in nft.c and pass it as parameter. All> >extensions will require this, and after that change you can return -1> >on error / 0 on success.> >> >Or simply pass the struct nft_rule object? Then, you can skip patch> >[libnftables PATCH 6/7]?> > Why not, it's a design preference. I liked the idea extension don't> mess up with the rule and only provides its expression list.> it's less code on libnftables on your idea at least.
We have to trust our iptables extensions.
What extra sanity checking are you going to make anyway if the
extension puzzles with this internal expr_list?
> >>+ .x6_options = DNAT_opts,> >>+ .translate_to_nft = DNAT_to_nft,> >nft_to_translate is missing, right? We need it to print the rule that> >is expressed in native format.> > Read the very first mail of this thread. It's actually an issue here.> I had the idea of doing the reverse function indeed, but there is a problem:> From iptables to nftables it's easy, since we get a target made by> the right extension, so we directly get the right translation> function. That's what I did.>> Now on the reverse way, we don't know at all to which extension the> expression list belongs to, so which translation function to call.> Currently the only way I see it is to loop on all extensions until> one returns successfully.
You need some dispatcher code that interprets the nft_expr and routes
it to the right iptables extension. So you will need also one .c file
per expression in the kernel, e.g. nft_nat.c, that performs this
dispatching / routing to the right extension.
Probably checking netlink_delinearize.c in nft can provide your some
ideas.
> We should take care of the position in the expression list as well,> and here I see we will need some more functions from libnftables.
You have the expression iterator already.
--
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

Hi Pablo,
>> Why not, it's a design preference. I liked the idea extension don't>> mess up with the rule and only provides its expression list.>> it's less code on libnftables on your idea at least.> We have to trust our iptables extensions.>> What extra sanity checking are you going to make anyway if the> extension puzzles with this internal expr_list?
What do you mean?
Return 0/-1 instead of pointer/NULL is same.
As I said I am fine with your proposal in addition that it requires less
code in libnftables.
>> Now on the reverse way, we don't know at all to which extension the>> expression list belongs to, so which translation function to call.>> Currently the only way I see it is to loop on all extensions until>> one returns successfully.> You need some dispatcher code that interprets the nft_expr and routes> it to the right iptables extension. So you will need also one .c file> per expression in the kernel, e.g. nft_nat.c, that performs this> dispatching / routing to the right extension.
You lost me. Why kernel is involved here?
> Probably checking netlink_delinearize.c in nft can provide your some> ideas.
Yes, and I actually use netlink_linearize.c to help for translation.
>> We should take care of the position in the expression list as well,>> and here I see we will need some more functions from libnftables.> You have the expression iterator already.
I believe it won't be sufficient. Let's see.
Tomasz
--
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 Wed, May 15, 2013 at 04:24:04PM +0300, Tomasz Bursztyka wrote:
[...]
> >>Now on the reverse way, we don't know at all to which extension the> >>expression list belongs to, so which translation function to call.> >>Currently the only way I see it is to loop on all extensions until> >>one returns successfully.>> >You need some dispatcher code that interprets the nft_expr and routes> >it to the right iptables extension. So you will need also one .c file> >per expression in the kernel, e.g. nft_nat.c, that performs this> >dispatching / routing to the right extension.> > You lost me. Why kernel is involved here?
I didn't mention the kernel is involved this.
You can have a dispatcher like:
static const struct {
const char *name;
void (*parse)(struct nft_rule_expr_iter *iter)
} netlink_parsers[] = {
[...]
{ .name = "nat", .parse = netlink_parse_nat },
};
the .parse callback gets an iterator to obtain the current expression
and munch more of them if required (will be useful for the payload
case).
Then, the netlink_parse_nat will route the nft_rule_expr object to the
corresponding libxt extension.
> >Probably checking netlink_delinearize.c in nft can provide your some> >ideas.> > Yes, and I actually use netlink_linearize.c to help for translation.> > >>We should take care of the position in the expression list as well,> >>and here I see we will need some more functions from libnftables.> >You have the expression iterator already.> > I believe it won't be sufficient. Let's see.
OK, let's revisit this once you hit limitations.
--
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