Hello,
On 10/12/16(Sat) 11:08, Dimitris Papastamos wrote:
> While I was playing around with the ip4 multicast code, I thought
> I would attempt to make the membership data structure similar to that
> of ip6. This means changing from a dynamic array to a linked list.
I like this a lot. Coherency is good.
> The max membership limit has been lifted (I did not see a similar limit
> in the ip6 code). I am not sure if this is appropriate for ip4 though.
> I can add the limit back if necessary.
I can't think of a reason why it was there in the first place.
> My system seems to work with this patch but I have not explicitly
> tested all the code paths I have modified. I am working on some small
> tests to prove that it works.
carp(4), pfsync(4) and vxlan(4) as well as ports using multicast like
avahi-daemon should be tested.
Some comments below.
> Index: sys/net/if_pfsync.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pfsync.c,v
> retrieving revision 1.238
> diff -u -p -r1.238 if_pfsync.c
> --- sys/net/if_pfsync.c 22 Nov 2016 19:29:54 -0000 1.238
> +++ sys/net/if_pfsync.c 10 Dec 2016 10:31:08 -0000
> @@ -313,10 +313,7 @@ pfsync_clone_create(struct if_clone *ifc
> sc->sc_len = PFSYNC_MINPKT;
> sc->sc_maxupdates = 128;
>
> - sc->sc_imo.imo_membership = (struct in_multi **)malloc(
> - (sizeof(struct in_multi *) * IP_MIN_MEMBERSHIPS), M_IPMOPTS,
> - M_WAITOK | M_ZERO);
> - sc->sc_imo.imo_max_memberships = IP_MIN_MEMBERSHIPS;
> + LIST_INIT(&sc->sc_imo.imo_memberships);
>
> ifp = &sc->sc_if;
> snprintf(ifp->if_xname, sizeof ifp->if_xname, "pfsync%d", unit);
> @@ -378,7 +375,6 @@ pfsync_clone_destroy(struct ifnet *ifp)
> }
>
> pool_destroy(&sc->sc_pool);
> - free(sc->sc_imo.imo_membership, M_IPMOPTS, 0);
> free(sc, M_DEVBUF, sizeof(*sc));
>
> pfsyncif = NULL;
> @@ -1319,9 +1315,11 @@ pfsyncioctl(struct ifnet *ifp, u_long cm
> sc->sc_sync_if->if_linkstatehooks,
> sc->sc_lhcookie);
> sc->sc_sync_if = NULL;
> - if (imo->imo_num_memberships > 0) {
> - in_delmulti(imo->imo_membership[
> - --imo->imo_num_memberships]);
> + if (!LIST_EMPTY(&imo->imo_memberships)) {
> + struct in_multi_mship *imm =
> + LIST_FIRST(&imo->imo_memberships);
> + LIST_REMOVE(imm, imm_chain);
> + in_leavegroup(imm);
> imo->imo_ifidx = 0;
> }
This is all correct but I'd suggest to keep the difference with other
multicast code as small as possible. If I take carp(4)'s example, it
becomes:
while (!LIST_EMPTY(&imo->imo_memberships)) {
struct in_multi_mship *imm =
LIST_FIRST(&imo->imo_memberships);
LIST_REMOVE(imm, imm_chain);
in_leavegroup(imm);
}
imo->imo_ifidx = 0;
> @@ -1345,13 +1343,17 @@ pfsyncioctl(struct ifnet *ifp, u_long cm
> sc->sc_lhcookie);
> sc->sc_sync_if = sifp;
>
> - if (imo->imo_num_memberships > 0) {
> -
> in_delmulti(imo->imo_membership[--imo->imo_num_memberships]);
> + if (!LIST_EMPTY(&imo->imo_memberships)) {
> + struct in_multi_mship *imm =
> + LIST_FIRST(&imo->imo_memberships);
> + LIST_REMOVE(imm, imm_chain);
> + in_leavegroup(imm);
> imo->imo_ifidx = 0;
> }
Same here ;)
> @@ -1362,16 +1364,15 @@ pfsyncioctl(struct ifnet *ifp, u_long cm
>
> addr.s_addr = INADDR_PFSYNC_GROUP;
>
> - if ((imo->imo_membership[0] =
> - in_addmulti(&addr, sc->sc_sync_if)) == NULL) {
> + if ((imm = in_joingroup(&addr, sc->sc_sync_if)) ==
> NULL) {
Isn't this line longer than 80 chars?
> Index: sys/netinet/in_var.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in_var.h,v
> retrieving revision 1.39
> diff -u -p -r1.39 in_var.h
> --- sys/netinet/in_var.h 15 Jun 2016 19:39:34 -0000 1.39
> +++ sys/netinet/in_var.h 10 Dec 2016 10:31:08 -0000
> @@ -123,6 +123,12 @@ struct in_multi {
> struct router_info *inm_rti; /* router version info */
> };
>
> +/* multicast membership entry */
> +struct in_multi_mship {
> + struct in_multi *imm_maddr; /* multicast address
> pointer */
> + LIST_ENTRY(in_multi_mship) imm_chain; /* multicast options
> chain */
> +};
Be careful about long lines! :)
> Index: sys/netinet/ip_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.331
> diff -u -p -r1.331 ip_output.c
> --- sys/netinet/ip_output.c 28 Nov 2016 10:14:00 -0000 1.331
> +++ sys/netinet/ip_output.c 10 Dec 2016 10:31:08 -0000
> @@ -1480,65 +1475,28 @@ ip_setmoptions(int optname, struct ip_mo
> break;
> }
> /*
> - * See if the membership already exists or if all the
> - * membership slots are full.
> + * See if the membership already exists.
> */
> - for (i = 0; i < imo->imo_num_memberships; ++i) {
> - if (imo->imo_membership[i]->inm_ifidx
> - == ifp->if_index &&
> - imo->imo_membership[i]->inm_addr.s_addr
> - == mreq->imr_multiaddr.s_addr)
> + LIST_FOREACH(imm, &imo->imo_memberships, imm_chain)
> + if (imm->imm_maddr->inm_ifidx == ifp->if_index &&
> + imm->imm_maddr->inm_addr.s_addr ==
> mreq->imr_multiaddr.s_addr)
Long line here too :)
> break;
> - }
> - if (i < imo->imo_num_memberships) {
> + if (imm != NULL) {
> error = EADDRINUSE;
> if_put(ifp);
> break;
> }
> - if (imo->imo_num_memberships == imo->imo_max_memberships) {
> - struct in_multi **nmships, **omships;
> - size_t newmax;
> - /*
> - * Resize the vector to next power-of-two minus 1. If
> the
> - * size would exceed the maximum then we know we've
> really
> - * run out of entries. Otherwise, we reallocate the
> vector.
> - */
> - nmships = NULL;
> - omships = imo->imo_membership;
> - newmax = ((imo->imo_max_memberships + 1) * 2) - 1;
> - if (newmax <= IP_MAX_MEMBERSHIPS) {
> - nmships = (struct in_multi **)malloc(
> - sizeof(*nmships) * newmax, M_IPMOPTS,
> - M_NOWAIT|M_ZERO);
> - if (nmships != NULL) {
> - memcpy(nmships, omships,
> - sizeof(*omships) *
> - imo->imo_max_memberships);
> - free(omships, M_IPMOPTS,
> - sizeof(*omships) *
> - imo->imo_max_memberships);
> - imo->imo_membership = nmships;
> - imo->imo_max_memberships = newmax;
> - }
> - }
> - if (nmships == NULL) {
> - error = ENOBUFS;
> - if_put(ifp);
> - break;
> - }
> - }
> /*
> * Everything looks good; add a new record to the multicast
> * address list for the given interface.
> */
> - if ((imo->imo_membership[i] =
> - in_addmulti(&mreq->imr_multiaddr, ifp)) == NULL) {
> + if ((imm = in_joingroup(&mreq->imr_multiaddr, ifp)) == NULL) {
> error = ENOBUFS;
> if_put(ifp);
> break;
> }
> - ++imo->imo_num_memberships;
> if_put(ifp);
> + LIST_INSERT_HEAD(&imo->imo_memberships, imm, imm_chain);
> break;
>
> case IP_DROP_MEMBERSHIP:
> @@ -1576,15 +1534,12 @@ ip_setmoptions(int optname, struct ip_mo
> /*
> * Find the membership in the membership array.
> */
> - for (i = 0; i < imo->imo_num_memberships; ++i) {
> + LIST_FOREACH(imm, &imo->imo_memberships, imm_chain)
> if ((ifp == NULL ||
> - imo->imo_membership[i]->inm_ifidx ==
> - ifp->if_index) &&
> - imo->imo_membership[i]->inm_addr.s_addr ==
> - mreq->imr_multiaddr.s_addr)
> + imm->imm_maddr->inm_ifidx == ifp->if_index) &&
> + imm->imm_maddr->inm_addr.s_addr ==
> mreq->imr_multiaddr.s_addr)
And here.
> break;
> - }
> - if (i == imo->imo_num_memberships) {
> + if (imm == NULL) {
> error = EADDRNOTAVAIL;
> break;
> }
