Download raw body.
convert IN_LOOKUP_MULTI to function
12.11.2025 02:07, Alexander Bluhm пишет:
> Hi,
>
> I would like to convert macros IN_LOOKUP_MULTI() and IN6_LOOKUP_MULTI()
> into functions. Has less side effects and makes future locking easier.
>
> ok?
OK kn with the comments tweaked, the rest is up to you.
>
> bluhm
>
> Index: netinet/igmp.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/igmp.c,v
> diff -u -p -r1.89 igmp.c
> --- netinet/igmp.c 11 Nov 2025 13:05:35 -0000 1.89
> +++ netinet/igmp.c 11 Nov 2025 18:48:07 -0000
> @@ -396,7 +396,7 @@ igmp_input_if(struct ifnet *ifp, struct
> * If we belong to the group being reported, stop
> * our timer for that group.
> */
> - IN_LOOKUP_MULTI(igmp->igmp_group, ifp, inm);
> + inm = in_lookupmulti(&igmp->igmp_group, ifp);
> if (inm != NULL) {
> inm->inm_timer = 0;
> igmpstat_inc(igps_rcv_ourreports);
> @@ -464,7 +464,7 @@ igmp_input_if(struct ifnet *ifp, struct
> * If we belong to the group being reported, stop
> * our timer for that group.
> */
> - IN_LOOKUP_MULTI(igmp->igmp_group, ifp, inm);
> + inm = in_lookupmulti(&igmp->igmp_group, ifp);
> if (inm != NULL) {
> inm->inm_timer = 0;
> igmpstat_inc(igps_rcv_ourreports);
> Index: netinet/in.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.c,v
> diff -u -p -r1.187 in.c
> --- netinet/in.c 8 Jul 2025 00:47:41 -0000 1.187
> +++ netinet/in.c 11 Nov 2025 18:48:07 -0000
> @@ -820,10 +820,33 @@ in_broadcast(struct in_addr in, u_int rt
> }
>
> /*
> + * Function for looking up the in_multi record for a given IP multicast
> + * address on a given interface. If no matching record is found, "inm"
> + * returns NULL.
No need to say it's a function, best use imperative tense (like elsewhere), e.g.
Look up the ...
Using "inm" reads odd; with the above, I'd just say something like
Return the matching record if found or NULL.
> + */
> +struct in_multi *
> +in_lookupmulti(struct in_addr *addr, struct ifnet *ifp)
addr should be const.
ifp could be as well despite the TAILQ access, if I'm not mistaken.
> +{
> + struct in_multi *inm = NULL;
> + struct ifmaddr *ifma;
> +
> + NET_ASSERT_LOCKED();
> +
> + TAILQ_FOREACH(ifma, &ifp->if_maddrlist, ifma_list) {
> + if (ifma->ifma_addr->sa_family == AF_INET &&
> + ifmatoinm(ifma)->inm_addr.s_addr == addr->s_addr) {
> + inm = ifmatoinm(ifma);
> + break;
ifmatoinm() is just a cast, so called twice seems fine, but once reads
better (to me) and is fine regardless of what the function does.
Assigned inside the loop, return directly and you get smaller scope
TAILQ_FOREACH(ifma, &ifp->if_maddrlist, ifma_list) {
struct in_multi *inm = ifmatoinm(ifma);
if (ifma->ifma_addr->sa_family == AF_INET &&
inm->inm_addr.s_addr == addr->s_addr)
return (inm);
}
}
return (NULL);
Although we probably don't want function calls in declarations;
the rest still stands, though.
> + }
> + }
> + return (inm);
> +}
> +
> +/*
> * Add an address to the list of IP multicast addresses for a given interface.
> */
> struct in_multi *
> -in_addmulti(struct in_addr *ap, struct ifnet *ifp)
> +in_addmulti(struct in_addr *addr, struct ifnet *ifp)
> {
> struct in_multi *inm;
> struct ifreq ifr;
> @@ -831,7 +854,7 @@ in_addmulti(struct in_addr *ap, struct i
> /*
> * See if address already in list.
> */
> - IN_LOOKUP_MULTI(*ap, ifp, inm);
> + inm = in_lookupmulti(addr, ifp);
> if (inm != NULL) {
> /*
> * Found it; just increment the reference count.
> @@ -845,7 +868,7 @@ in_addmulti(struct in_addr *ap, struct i
> inm = malloc(sizeof(*inm), M_IPMADDR, M_WAITOK | M_ZERO);
> inm->inm_sin.sin_len = sizeof(struct sockaddr_in);
> inm->inm_sin.sin_family = AF_INET;
> - inm->inm_sin.sin_addr = *ap;
> + inm->inm_sin.sin_addr = *addr;
> refcnt_init_trace(&inm->inm_refcnt, DT_REFCNT_IDX_IFMADDR);
> inm->inm_ifidx = ifp->if_index;
> inm->inm_ifma.ifma_addr = sintosa(&inm->inm_sin);
> @@ -918,16 +941,16 @@ in_delmulti(struct in_multi *inm)
> }
>
> /*
> - * Return 1 if the multicast group represented by ``ap'' has been
> + * Return 1 if the multicast group represented by ``addr'' has been
> * joined by interface ``ifp'', 0 otherwise.
> */
> int
> -in_hasmulti(struct in_addr *ap, struct ifnet *ifp)
> +in_hasmulti(struct in_addr *addr, struct ifnet *ifp)
Likewise, addr/ifp should/can be const.
> {
> struct in_multi *inm;
Can be const.
> int joined;
>
> - IN_LOOKUP_MULTI(*ap, ifp, inm);
> + inm = in_lookupmulti(addr, ifp);
> joined = (inm != NULL);
>
> return (joined);
Or you just fold all this into, imho, equally readable
return (in_lookupmulti(addr, ifp) != NULL);
Then it's also obvious that their signutes must match.
> Index: netinet/in_var.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_var.h,v
> diff -u -p -r1.42 in_var.h
> --- netinet/in_var.h 11 Nov 2025 13:05:35 -0000 1.42
> +++ netinet/in_var.h 11 Nov 2025 18:48:07 -0000
> @@ -119,30 +119,9 @@ ifmatoinm(struct ifmaddr *ifma)
> return ((struct in_multi *)(ifma));
> }
>
> -/*
> - * Macro for looking up the in_multi record for a given IP multicast
> - * address on a given interface. If no matching record is found, "inm"
> - * returns NULL.
> - */
> -#define IN_LOOKUP_MULTI(addr, ifp, inm) \
> - /* struct in_addr addr; */ \
> - /* struct ifnet *ifp; */ \
> - /* struct in_multi *inm; */ \
> -do { \
> - struct ifmaddr *ifma; \
> - \
> - (inm) = NULL; \
> - NET_ASSERT_LOCKED(); \
> - TAILQ_FOREACH(ifma, &(ifp)->if_maddrlist, ifma_list) \
> - if (ifma->ifma_addr->sa_family == AF_INET && \
> - ifmatoinm(ifma)->inm_addr.s_addr == (addr).s_addr) {\
> - (inm) = ifmatoinm(ifma); \
> - break; \
> - } \
> -} while (/* CONSTCOND */ 0)
> -
> int in_ifinit(struct ifnet *,
> struct in_ifaddr *, struct sockaddr_in *, int);
> +struct in_multi *in_lookupmulti(struct in_addr *, struct ifnet *);
> struct in_multi *in_addmulti(struct in_addr *, struct ifnet *);
> void in_delmulti(struct in_multi *);
> int in_hasmulti(struct in_addr *, struct ifnet *);
Same for netinet6/.
convert IN_LOOKUP_MULTI to function