Hi Paolo
Thanks for the review feedback, I will adjust them in the patch v6.
>Why moving the struct definition here? IMHO addrconf.h is better suited
>and will avoid additional headers dep.
The `struct inet_fill_args` is moved from devinet.c to igmp.h to make
it usable in both devinet.c and igmp.c.
I feel it is incorrect to move `struct inet_fill_args` to addrconf.h
because that file contains IPv6 specific definition and it also
contains `struct inet6_fill_args`. After refactoring, I will remove
the usage of enum addr_type_t type, so we don't need to import
addrconf.h anymore.
Thanks,
Yuyang
On Thu, Jan 16, 2025 at 8:06 PM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
>
> On 1/14/25 3:37 AM, Yuyang Huang wrote:
> > Extended RTM_GETMULTICAST to support dumping joined IPv4 multicast
> > addresses, in addition to the existing IPv6 functionality. This allows
> > userspace applications to retrieve both IPv4 and IPv6 multicast
> > addresses through similar netlink command and then monitor future
> > changes by registering to RTNLGRP_IPV4_MCADDR and RTNLGRP_IPV6_MCADDR.
> >
> > This change includes a new ynl based selftest case. To run the test,
> > execute the following command:
> >
> > $ vng -v --user root --cpus 16 -- \
> > make -C tools/testing/selftests TARGETS=net \
> > TEST_PROGS=rtnetlink.py TEST_GEN_PROGS="" run_tests
>
> Thanks for including the test!
>
> > diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml
> > index 0d492500c7e5..7dcd5fddac9d 100644
> > --- a/Documentation/netlink/specs/rt_link.yaml
> > +++ b/Documentation/netlink/specs/rt_link.yaml
> > @@ -92,6 +92,41 @@ definitions:
> > -
> > name: ifi-change
> > type: u32
> > + -
> > + name: ifaddrmsg
> > + type: struct
> > + members:
> > + -
> > + name: ifa-family
> > + type: u8
> > + -
> > + name: ifa-prefixlen
> > + type: u8
> > + -
> > + name: ifa-flags
> > + type: u8
> > + -
> > + name: ifa-scope
> > + type: u8
> > + -
> > + name: ifa-index
> > + type: u32
> > + -
> > + name: ifacacheinfo
> > + type: struct
> > + members:
> > + -
> > + name: ifa-prefered
> > + type: u32
> > + -
> > + name: ifa-valid
> > + type: u32
> > + -
> > + name: cstamp
> > + type: u32
> > + -
> > + name: tstamp
> > + type: u32
> > -
> > name: ifla-bridge-id
> > type: struct
> > @@ -2253,6 +2288,18 @@ attribute-sets:
> > -
> > name: tailroom
> > type: u16
> > + -
> > + name: ifmcaddr-attrs
> > + attributes:
> > + -
> > + name: addr
> > + type: binary
> > + value: 7
> > + -
> > + name: cacheinfo
> > + type: binary
> > + struct: ifacacheinfo
> > + value: 6
> >
> > sub-messages:
> > -
> > @@ -2493,6 +2540,29 @@ operations:
> > reply:
> > value: 92
> > attributes: *link-stats-attrs
> > + -
> > + name: getmaddrs
> > + doc: Get / dump IPv4/IPv6 multicast addresses.
> > + attribute-set: ifmcaddr-attrs
> > + fixed-header: ifaddrmsg
> > + do:
> > + request:
> > + value: 58
> > + attributes:
> > + - ifa-family
> > + - ifa-index
> > + reply:
> > + value: 58
> > + attributes: &mcaddr-attrs
> > + - addr
> > + - cacheinfo
> > + dump:
> > + request:
> > + value: 58
> > + - ifa-family
> > + reply:
> > + value: 58
> > + attributes: *mcaddr-attrs
> >
> > mcast-groups:
> > list:
>
> IMHO the test case itself should land to a separate patch.
>
> > diff --git a/include/linux/igmp.h b/include/linux/igmp.h
> > index 073b30a9b850..a460e1ef0524 100644
> > --- a/include/linux/igmp.h
> > +++ b/include/linux/igmp.h
> > @@ -16,6 +16,7 @@
> > #include <linux/ip.h>
> > #include <linux/refcount.h>
> > #include <linux/sockptr.h>
> > +#include <net/addrconf.h>
> > #include <uapi/linux/igmp.h>
> >
> > static inline struct igmphdr *igmp_hdr(const struct sk_buff *skb)
> > @@ -92,6 +93,16 @@ struct ip_mc_list {
> > struct rcu_head rcu;
> > };
> >
> > +struct inet_fill_args {
> > + u32 portid;
> > + u32 seq;
> > + int event;
> > + unsigned int flags;
> > + int netnsid;
> > + int ifindex;
> > + enum addr_type_t type;
> > +};
>
> Why moving the struct definition here? IMHO addrconf.h is better suited
> and will avoid additional headers dep.
>
> > @@ -1874,6 +1894,23 @@ static int in_dev_dump_addr(struct in_device *in_dev, struct sk_buff *skb,
> > return err;
> > }
> >
> > +static int in_dev_dump_addr(struct in_device *in_dev, struct sk_buff *skb,
> > + struct netlink_callback *cb, int *s_ip_idx,
> > + struct inet_fill_args *fillargs)
> > +{
> > + switch (fillargs->type) {
> > + case UNICAST_ADDR:
> > + fillargs->event = RTM_NEWADDR;
>
> I think that adding an additional argument for 'event' to
> inet_dump_addr() would simplify the code a bit.
>
> > + return in_dev_dump_ifaddr(in_dev, skb, cb, s_ip_idx, fillargs);
> > + case MULTICAST_ADDR:
> > + fillargs->event = RTM_GETMULTICAST;
> > + return in_dev_dump_ifmcaddr(in_dev, skb, cb, s_ip_idx,
> > + fillargs);
> > + default:
> > + return 0;
>
> Why not erroring out on unknown types? should never happen, right?
>
> > @@ -1949,6 +1987,20 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > return err;
> > }
> >
> > +static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > +{
> > + enum addr_type_t type = UNICAST_ADDR;
> > +
> > + return inet_dump_addr(skb, cb, type);
>
> You can avoid the local variable usage.
>
> > +}
> > +
> > +static int inet_dump_ifmcaddr(struct sk_buff *skb, struct netlink_callback *cb)
> > +{
> > + enum addr_type_t type = MULTICAST_ADDR;
> > +
> > + return inet_dump_addr(skb, cb, type);
>
> Same here.
>
> Thanks!
>
> Paolo
>
