Hi,
On Fri, 2013-03-15 at 13:50 +0100, Daniel Wagner wrote:
> From: Daniel Wagner <[email protected]>
>
> prepare_rule_inclusion() is currently working correct for APPEND
> operations.
>
> First let's have a look what it does currently:
>
> - user chains will be ignored by this change, because it head->builtin
> flag is always -1.
> - For builtin chains there are two cases to look at
> -- chain is emtpy: In this case chain_head == chain_tail->prev holds true
> and we want to 'append' the new rule as chain head right before
> the policy rule which marks the end of the chain.
> -- chain is not empty: then we don't have to handle the builtin flag
> update. The only caller is iptables_append_rule which will put the
> new rule before chain_tail->prev anyway.
>
> The next patch brings back iptables_insert_rule() (commit 161efbae1
> removed it) but this function will not work for non empty chain. In
> this case the condition 'chain_head == chain_tail->prev' is false and
> therefore we will not tell via *builtin that this is the new
> head. Instead iptables_insert_rule() will insert the new rule at the
> second position (see if (builtin == -1) chain_head = chain_head->next)
>
> Therefore we need to tell prepare_rule_inclusion() that the new rule
> is always at first position.
Now I start to get it. The commit message above is an almost there, but
not quite. The commit subject line should say something like "Add
boolean variable to distinguish between insert and append". The first
thing to describe is the new variable, then why chain_head ==
chain_tail->prev is still tested. And no sneak peeks on the next patch -
it's confusing :-)
> ---
> src/iptables.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/iptables.c b/src/iptables.c
> index 1fd6c42..8454fcb 100644
> --- a/src/iptables.c
> +++ b/src/iptables.c
> @@ -768,7 +768,8 @@ static struct ipt_entry *prepare_rule_inclusion(struct
> connman_iptables *table,
> struct ipt_ip *ip, const char *chain_name,
> const char *target_name,
> struct xtables_target *xt_t,
> - int *builtin, struct xtables_rule_match *xt_rm)
> + int *builtin, struct xtables_rule_match *xt_rm,
> + connman_bool_t insert_first)
I think 'insert_first' is misleading as a variable name. As we don't
have indexes, the preparation is always for an insert or append
function. Just rename it to plain 'insert' with a more on the spot
commit message and we're done with this one.
> {
> GList *chain_tail, *chain_head;
> struct ipt_entry *new_entry;
> @@ -796,7 +797,7 @@ static struct ipt_entry *prepare_rule_inclusion(struct
> connman_iptables *table,
> head = chain_head->data;
> if (head->builtin < 0)
> *builtin = -1;
> - else if (chain_head == chain_tail->prev) {
> + else if (insert_first == TRUE || chain_head == chain_tail->prev) {
> *builtin = head->builtin;
> head->builtin = -1;
> }
> @@ -821,7 +822,7 @@ static int iptables_append_rule(struct connman_iptables
> *table,
> return -EINVAL;
>
> new_entry = prepare_rule_inclusion(table, ip, chain_name,
> - target_name, xt_t, &builtin, xt_rm);
> + target_name, xt_t, &builtin, xt_rm, FALSE);
> if (new_entry == NULL)
> return -EINVAL;
>
Cheers,
Patrik
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
