| Message ID | 1372376687.21767.10.camel@imac-linux.luckyscavenger.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jun 27, 2013 at 04:44:47PM -0700, Sven-Thorsten Dietrich wrote: > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > index dfc39d4..695858b 100644 > --- a/net/ipv4/devinet.c > +++ b/net/ipv4/devinet.c > @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex, > return -EMSGSIZE; > > ncm = nlmsg_data(nlh); > - ncm->ncm_family = AF_INET; > + > + switch (type) { > + case NETCONFA_MC_FORWARDING: > + ncm->ncm_family = RTNL_FAMILY_IPMR; > + break; > + > + default: > + ncm->ncm_family = AF_INET; > + break; > + } > > if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0) > goto nla_put_failure; > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 4ab4c38..a177da4 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex, > return -EMSGSIZE; > > ncm = nlmsg_data(nlh); > - ncm->ncm_family = AF_INET6; > + > + switch (type) { > + case NETCONFA_MC_FORWARDING: > + ncm->ncm_family = RTNL_FAMILY_IP6MR; > + break; > + > + default: > + ncm->ncm_family = AF_INET6; > + break; > + } > Hm, are you sure? NETCONFA_MC_FORWARDING is of type RTM_NEWNETCONF and expects ncm_family to be either AF_INET or AF_INET6 (at least in iproute2/ipmonitor.c). Greetings, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 28 Jun 2013 03:23:07 +0200 Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Thu, Jun 27, 2013 at 04:44:47PM -0700, Sven-Thorsten Dietrich wrote: > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > > index dfc39d4..695858b 100644 > > --- a/net/ipv4/devinet.c > > +++ b/net/ipv4/devinet.c > > @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex, > > return -EMSGSIZE; > > > > ncm = nlmsg_data(nlh); > > - ncm->ncm_family = AF_INET; > > + > > + switch (type) { > > + case NETCONFA_MC_FORWARDING: > > + ncm->ncm_family = RTNL_FAMILY_IPMR; > > + break; > > + > > + default: > > + ncm->ncm_family = AF_INET; > > + break; > > + } > > > > if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0) > > goto nla_put_failure; > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > > index 4ab4c38..a177da4 100644 > > --- a/net/ipv6/addrconf.c > > +++ b/net/ipv6/addrconf.c > > @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex, > > return -EMSGSIZE; > > > > ncm = nlmsg_data(nlh); > > - ncm->ncm_family = AF_INET6; > > + > > + switch (type) { > > + case NETCONFA_MC_FORWARDING: > > + ncm->ncm_family = RTNL_FAMILY_IP6MR; > > + break; > > + > > + default: > > + ncm->ncm_family = AF_INET6; > > + break; > > + } > > > > Hm, are you sure? NETCONFA_MC_FORWARDING is of type RTM_NEWNETCONF > and expects ncm_family to be either AF_INET or AF_INET6 (at least in > iproute2/ipmonitor.c). > I agree with Sven on this, looks like the recent addition of netconf configuration to netlink didn't embrace how multicast is handled in kernel. Multicast forwarding is a routing related configuration value. All the multicast routing events come in as special family RTNL_FAMILY_IPMR (see net/ipv4/ipmr.c function ipmr_fill_route). I would expect that multicast routing daemons would like to be able to use special family to listen for all multicast related changes (and not see non-multicast events). Minor nit: the patch is formatted incorrectly (case should line up with switch). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 27, 2013 at 06:33:42PM -0700, Stephen Hemminger wrote: > On Fri, 28 Jun 2013 03:23:07 +0200 > Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > Hm, are you sure? NETCONFA_MC_FORWARDING is of type RTM_NEWNETCONF > > and expects ncm_family to be either AF_INET or AF_INET6 (at least in > > iproute2/ipmonitor.c). > > > > I agree with Sven on this, looks like the recent addition of netconf > configuration to netlink didn't embrace how multicast is handled in kernel. > > Multicast forwarding is a routing related configuration value. > All the multicast routing events come in as special family RTNL_FAMILY_IPMR > (see net/ipv4/ipmr.c function ipmr_fill_route). I would expect that multicast > routing daemons would like to be able to use special family to listen for > all multicast related changes (and not see non-multicast events). > > > Minor nit: the patch is formatted incorrectly (case should line up with switch). Yes, this seems reasonable but would need a small update to ipnetconf.c, too. Thanks, Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 28/06/2013 03:51, Hannes Frederic Sowa a écrit : > On Thu, Jun 27, 2013 at 06:33:42PM -0700, Stephen Hemminger wrote: >> On Fri, 28 Jun 2013 03:23:07 +0200 >> Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: >>> Hm, are you sure? NETCONFA_MC_FORWARDING is of type RTM_NEWNETCONF >>> and expects ncm_family to be either AF_INET or AF_INET6 (at least in >>> iproute2/ipmonitor.c). >>> >> >> I agree with Sven on this, looks like the recent addition of netconf >> configuration to netlink didn't embrace how multicast is handled in kernel. >> >> Multicast forwarding is a routing related configuration value. >> All the multicast routing events come in as special family RTNL_FAMILY_IPMR >> (see net/ipv4/ipmr.c function ipmr_fill_route). I would expect that multicast >> routing daemons would like to be able to use special family to listen for >> all multicast related changes (and not see non-multicast events). >> >> >> Minor nit: the patch is formatted incorrectly (case should line up with switch). > > Yes, this seems reasonable but would need a small update to ipnetconf.c, too. I also agree with Sven and Stephen. Note also that the Signed-off-by line is missing in the commit log. Sven, can you take care of the iproute2 patch? If not, let me know so I can do it. Thank you, Nicolas -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 27 Jun 2013 16:44:47 -0700 Sven-Thorsten Dietrich <sven@vyatta.com> wrote: > There may be other cases that require a special case, so I chose the switch approach. > > Please note, that all other messages from ipmr[6].c correctly set the family, while these do not. > > Thanks > > Sven > > > > Subject: Set correct RTNL family for multicast netconf messages > From: Sven-Thorsten Dietrich sven@vyatta.com Thu Jun 27 16:40:17 2013 -0700 > Date: Thu Jun 27 16:40:17 2013 -0700: > > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > index dfc39d4..695858b 100644 > --- a/net/ipv4/devinet.c > +++ b/net/ipv4/devinet.c > @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex, > return -EMSGSIZE; > > ncm = nlmsg_data(nlh); > - ncm->ncm_family = AF_INET; > + > + switch (type) { > + case NETCONFA_MC_FORWARDING: > + ncm->ncm_family = RTNL_FAMILY_IPMR; > + break; > + > + default: > + ncm->ncm_family = AF_INET; > + break; > + } > > if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0) > goto nla_put_failure; > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 4ab4c38..a177da4 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex, > return -EMSGSIZE; > > ncm = nlmsg_data(nlh); > - ncm->ncm_family = AF_INET6; > + > + switch (type) { > + case NETCONFA_MC_FORWARDING: > + ncm->ncm_family = RTNL_FAMILY_IP6MR; > + break; > + > + default: > + ncm->ncm_family = AF_INET6; > + break; > + } > > if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0) > goto nla_put_failure; > Found another issue that needs some thought. If type == ALL, in that case the family comes up as AF_INET. That means that if application is doing a request to get netconf it will receive a different answer than if it is montoring for netconf changes. One way to solve would be to split fill_devconf into two parts, one for unicast, and one for multicast. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 28/06/2013 17:26, Stephen Hemminger a écrit : > On Thu, 27 Jun 2013 16:44:47 -0700 > Sven-Thorsten Dietrich <sven@vyatta.com> wrote: > >> There may be other cases that require a special case, so I chose the switch approach. >> >> Please note, that all other messages from ipmr[6].c correctly set the family, while these do not. >> >> Thanks >> >> Sven >> >> >> >> Subject: Set correct RTNL family for multicast netconf messages >> From: Sven-Thorsten Dietrich sven@vyatta.com Thu Jun 27 16:40:17 2013 -0700 >> Date: Thu Jun 27 16:40:17 2013 -0700: >> >> >> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c >> index dfc39d4..695858b 100644 >> --- a/net/ipv4/devinet.c >> +++ b/net/ipv4/devinet.c >> @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex, >> return -EMSGSIZE; >> >> ncm = nlmsg_data(nlh); >> - ncm->ncm_family = AF_INET; >> + >> + switch (type) { >> + case NETCONFA_MC_FORWARDING: >> + ncm->ncm_family = RTNL_FAMILY_IPMR; >> + break; >> + >> + default: >> + ncm->ncm_family = AF_INET; >> + break; >> + } >> >> if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0) >> goto nla_put_failure; >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index 4ab4c38..a177da4 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex, >> return -EMSGSIZE; >> >> ncm = nlmsg_data(nlh); >> - ncm->ncm_family = AF_INET6; >> + >> + switch (type) { >> + case NETCONFA_MC_FORWARDING: >> + ncm->ncm_family = RTNL_FAMILY_IP6MR; >> + break; >> + >> + default: >> + ncm->ncm_family = AF_INET6; >> + break; >> + } >> >> if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0) >> goto nla_put_failure; >> > > Found another issue that needs some thought. > If type == ALL, in that case the family comes up as AF_INET. > That means that if application is doing a request to get netconf > it will receive a different answer than if it is montoring for netconf > changes. > > One way to solve would be to split fill_devconf into two parts, one > for unicast, and one for multicast. If I understand well, to get all conf variables for IPv4, you will need to make two dump? Note that the initial idea of netconf was to be able to dump via netlink the content of /proc/sys/net/ipv4/conf/ or /proc/sys/net/ipv6/conf/, hence AF_INET[6] was used to specify 'ipv4' or 'ipv6'. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index dfc39d4..695858b 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex, return -EMSGSIZE; ncm = nlmsg_data(nlh); - ncm->ncm_family = AF_INET; + + switch (type) { + case NETCONFA_MC_FORWARDING: + ncm->ncm_family = RTNL_FAMILY_IPMR; + break; + + default: + ncm->ncm_family = AF_INET; + break; + } if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0) goto nla_put_failure; diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 4ab4c38..a177da4 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex, return -EMSGSIZE; ncm = nlmsg_data(nlh); - ncm->ncm_family = AF_INET6; + + switch (type) { + case NETCONFA_MC_FORWARDING: + ncm->ncm_family = RTNL_FAMILY_IP6MR; + break; + + default: + ncm->ncm_family = AF_INET6; + break; + } if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0) goto nla_put_failure;
There may be other cases that require a special case, so I chose the switch approach. Please note, that all other messages from ipmr[6].c correctly set the family, while these do not. Thanks Sven Subject: Set correct RTNL family for multicast netconf messages From: Sven-Thorsten Dietrich sven@vyatta.com Thu Jun 27 16:40:17 2013 -0700 Date: Thu Jun 27 16:40:17 2013 -0700: