On 06/17/2015 07:50 AM, roopa wrote:
On 6/17/15, 12:50 AM, Scott Feldman wrote:
On Tue, Jun 16, 2015 at 9:11 AM, Roopa Prabhu
<ro...@cumulusnetworks.com> wrote:
[snip]
@@ -1203,6 +1204,8 @@ int fib_table_insert(struct fib_table *tb,
struct fib_config *cfg)
if (!(cfg->fc_nlflags & NLM_F_APPEND))
fa = fa_first;
+ else
+ nlflags |= NLM_F_APPEND;
}
The if and else parts above don't seem logically related.
I have it at this place because here is where the decision to append
really happens.
Maybe you
could initialize nlflags as:
unsigned int nlflags = cfg->fc_nlflags &
(NLM_F_REPLACE|NLM_F_APPEND);
And then pass rtmsg_fib(..., nlflags) to avoid the flag test/set?
nlflags should only contain NLM_F_REPLACE and NLM_F_APPEND if a replace or
append really took place. Hence the check and setting of nlflags is at
the place where that
decision is made.
I had tried this patch a couple of other ways.... Do you think the below
would be less confusing ?
thanks.
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 3c699c4..9bfa3d8 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1082,6 +1082,7 @@ int fib_table_insert(struct fib_table *tb, struct
fib_config *cfg)
struct trie *t = (struct trie *)tb->tb_data;
struct fib_alias *fa, *new_fa;
struct key_vector *l, *tp;
+ unsigned int nlflags = 0;
struct fib_info *fi;
u8 plen = cfg->fc_dst_len;
u8 slen = KEYLENGTH - plen;
@@ -1189,8 +1190,9 @@ int fib_table_insert(struct fib_table *tb, struct
fib_config *cfg)
fib_release_info(fi_drop);
if (state & FA_S_ACCESSED)
rt_cache_flush(cfg->fc_nlinfo.nl_net);
+ nlflags |= NLM_F_REPLACE;
rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen,
- tb->tb_id, &cfg->fc_nlinfo, NLM_F_REPLACE);
+ tb->tb_id, &cfg->fc_nlinfo, nlflags);
goto succeeded;
Why even bother modifying this part? Is this actually needed at all,
are there some other flags you plan to drop into nlflags as well that
would be passed as a part of this message?
@@ -1201,7 +1203,9 @@ int fib_table_insert(struct fib_table *tb, struct
fib_config *cfg)
if (fa_match)
goto out;
- if (!(cfg->fc_nlflags & NLM_F_APPEND))
+ if (cfg->fc_nlflags & NLM_F_APPEND)
+ nlflags |= NLM_F_APPEND;
+ else
fa = fa_first;
}
err = -ENOENT;
I'm not sure I see the point of using the |=. Why not just use a = and
save yourself an instruction or two since you don't really need the OR
operator in this case.
@@ -1238,7 +1242,7 @@ int fib_table_insert(struct fib_table *tb, struct
fib_config *cfg)
rt_cache_flush(cfg->fc_nlinfo.nl_net);
rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, new_fa->tb_id,
- &cfg->fc_nlinfo, 0);
+ &cfg->fc_nlinfo, nlflags);
succeeded:
return 0;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
