* [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications @ 2015-06-16 16:11 Roopa Prabhu 2015-06-16 16:14 ` roopa 2015-06-17 7:50 ` Scott Feldman 0 siblings, 2 replies; 8+ messages in thread From: Roopa Prabhu @ 2015-06-16 16:11 UTC (permalink / raw) To: davem; +Cc: netdev From: Roopa Prabhu <roopa@cumulusnetworks.com> This patch adds NLM_F_APPEND flag to struct nlmsg_hdr->nlmsg_flags in newroute notifications if the route add was an append. (This is similar to how NLM_F_REPLACE is already part of new route replace notifications today) This helps userspace determine if the route add operation was an append. Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> --- net/ipv4/fib_trie.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c index 3c699c4..38ffc20 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; @@ -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; } err = -ENOENT; if (!(cfg->fc_nlflags & NLM_F_CREATE)) @@ -1238,7 +1241,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; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications 2015-06-16 16:11 [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications Roopa Prabhu @ 2015-06-16 16:14 ` roopa 2015-06-17 7:50 ` Scott Feldman 1 sibling, 0 replies; 8+ messages in thread From: roopa @ 2015-06-16 16:14 UTC (permalink / raw) To: davem; +Cc: netdev On 6/16/15, 9:11 AM, Roopa Prabhu wrote: > From: Roopa Prabhu <roopa@cumulusnetworks.com> > > This patch adds NLM_F_APPEND flag to struct nlmsg_hdr->nlmsg_flags > in newroute notifications if the route add was an append. > (This is similar to how NLM_F_REPLACE is already part of new > route replace notifications today) > > This helps userspace determine if the route add operation was > an append. > > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> please ignore, I plan to resend the patch against net-next ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications 2015-06-16 16:11 [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications Roopa Prabhu 2015-06-16 16:14 ` roopa @ 2015-06-17 7:50 ` Scott Feldman 2015-06-17 14:50 ` roopa 1 sibling, 1 reply; 8+ messages in thread From: Scott Feldman @ 2015-06-17 7:50 UTC (permalink / raw) To: Roopa Prabhu; +Cc: David S. Miller, Netdev On Tue, Jun 16, 2015 at 9:11 AM, Roopa Prabhu <roopa@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. 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications 2015-06-17 7:50 ` Scott Feldman @ 2015-06-17 14:50 ` roopa 2015-06-17 15:35 ` Alexander Duyck 0 siblings, 1 reply; 8+ messages in thread From: roopa @ 2015-06-17 14:50 UTC (permalink / raw) To: Scott Feldman; +Cc: David S. Miller, Netdev On 6/17/15, 12:50 AM, Scott Feldman wrote: > On Tue, Jun 16, 2015 at 9:11 AM, Roopa Prabhu <roopa@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; } @@ -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; @@ -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; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications 2015-06-17 14:50 ` roopa @ 2015-06-17 15:35 ` Alexander Duyck 2015-06-17 16:20 ` roopa 0 siblings, 1 reply; 8+ messages in thread From: Alexander Duyck @ 2015-06-17 15:35 UTC (permalink / raw) To: roopa, Scott Feldman; +Cc: David S. Miller, Netdev 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 >> <roopa@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; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications 2015-06-17 15:35 ` Alexander Duyck @ 2015-06-17 16:20 ` roopa 2015-06-17 17:31 ` Alexander Duyck 0 siblings, 1 reply; 8+ messages in thread From: roopa @ 2015-06-17 16:20 UTC (permalink / raw) To: Alexander Duyck; +Cc: Scott Feldman, David S. Miller, Netdev On 6/17/15, 8:35 AM, Alexander Duyck wrote: > >> @@ -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? agreed, for the same reason my initial patch did not touch this part. Nope, no other flags. I was trying to meet scotts concerns. > >> @@ -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. > ack, I would prefer keeping my initial patch which was pretty non-intrusive. thanks, Roopa ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications 2015-06-17 16:20 ` roopa @ 2015-06-17 17:31 ` Alexander Duyck 2015-06-17 18:07 ` roopa 0 siblings, 1 reply; 8+ messages in thread From: Alexander Duyck @ 2015-06-17 17:31 UTC (permalink / raw) To: roopa, Alexander Duyck; +Cc: Scott Feldman, David S. Miller, Netdev On 06/17/2015 09:20 AM, roopa wrote: > On 6/17/15, 8:35 AM, Alexander Duyck wrote: >> >>> @@ -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? > > agreed, for the same reason my initial patch did not touch this part. > Nope, no other flags. I was trying to meet scotts concerns. >> >>> @@ -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. >> > ack, > > I would prefer keeping my initial patch which was pretty non-intrusive. I'd say go with something closer to the original patch, but flip the logic like you have here, and lose the "|=" in favor of an "=" since you are either sending a message with 0 or NLM_F_APPEND. Anyway that is just my $.02. - Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications 2015-06-17 17:31 ` Alexander Duyck @ 2015-06-17 18:07 ` roopa 0 siblings, 0 replies; 8+ messages in thread From: roopa @ 2015-06-17 18:07 UTC (permalink / raw) To: Alexander Duyck; +Cc: Alexander Duyck, Scott Feldman, David S. Miller, Netdev On 6/17/15, 10:31 AM, Alexander Duyck wrote: > > I'd say go with something closer to the original patch, but flip the > logic like you have here, and lose the "|=" in favor of an "=" since > you are either sending a message with 0 or NLM_F_APPEND. > > Anyway that is just my $.02. agreed, thanks alex, v2 posted.. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-06-17 18:07 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-06-16 16:11 [PATCH net] ipv4: include NLM_F_APPEND flag in append route notifications Roopa Prabhu 2015-06-16 16:14 ` roopa 2015-06-17 7:50 ` Scott Feldman 2015-06-17 14:50 ` roopa 2015-06-17 15:35 ` Alexander Duyck 2015-06-17 16:20 ` roopa 2015-06-17 17:31 ` Alexander Duyck 2015-06-17 18:07 ` roopa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).