* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.