From owner-freebsd-net Sat Apr 6 17: 0:34 2002 Delivered-To: freebsd-net@freebsd.org Received: from rwcrmhc54.attbi.com (rwcrmhc54.attbi.com [216.148.227.87]) by hub.freebsd.org (Postfix) with ESMTP id 7D80237B405 for ; Sat, 6 Apr 2002 17:00:10 -0800 (PST) Received: from InterJet.elischer.org ([12.232.206.8]) by rwcrmhc54.attbi.com (InterMail vM.4.01.03.27 201-229-121-127-20010626) with ESMTP id <20020407010009.ENUD15826.rwcrmhc54.attbi.com@InterJet.elischer.org>; Sun, 7 Apr 2002 01:00:09 +0000 Received: from localhost (localhost.elischer.org [127.0.0.1]) by InterJet.elischer.org (8.9.1a/8.9.1) with ESMTP id QAA43452; Sat, 6 Apr 2002 16:44:33 -0800 (PST) Date: Sat, 6 Apr 2002 16:44:32 -0800 (PST) From: Julian Elischer To: Brooks Davis Cc: net@freebsd.org Subject: Re: review request: minor cloning API change In-Reply-To: <20020405230719.A13516@Odin.AC.HMC.Edu> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org Please excuse the comment if I'm way off the mark.. With a VERY BRIEF look I see that you are returning a void from the destroy function and it is callled from the module unload code where some destroy functions used to return ints. this ia I think a BAD MOVE.. a driver must be able to veto it's own unloading. If it is in use fro example. You leave no way for the driver to say "Nope, I can't be unloaded now, I'm busy." On Fri, 5 Apr 2002, Brooks Davis wrote: > The following patch reverts a previous API change which change the > return value of a clonable interfaces' destory function from void to > int to allow the interface to refuse to delete a unit. Since we now > manage unit creation in the generic cloning code and the only use mux or > I could thing of for refusing to delete a unit was forcing a certain > number of units to exist, I've added a new member to the cloner struct, > ifc_minifs which specifies the minimum number of units of this device > allowed. This changes the initilizer macro, but we already differ from > NetBSD in that area and we get to revert to function signatures that > match those from NetBSD in exchange. > > This diff also includes code to convert the disc interface to be > clonable and unloadable. This will be commited seperatly. > > -- Brooks > > > Index: if.c > =================================================================== > RCS file: /usr/cvs/src/sys/net/if.c,v > retrieving revision 1.137 > diff -u -p -r1.137 if.c > --- if.c 4 Apr 2002 21:03:28 -0000 1.137 > +++ if.c 6 Apr 2002 06:38:02 -0000 > @@ -656,12 +656,15 @@ if_clone_destroy(name) > struct if_clone *ifc; > struct ifnet *ifp; > int bytoff, bitoff; > - int err, unit; > + int unit; > > ifc = if_clone_lookup(name, &unit); > if (ifc == NULL) > return (EINVAL); > > + if (unit < ifc->ifc_minifs) > + return (EINVAL); > + > ifp = ifunit(name); > if (ifp == NULL) > return (ENXIO); > @@ -669,9 +672,7 @@ if_clone_destroy(name) > if (ifc->ifc_destroy == NULL) > return (EOPNOTSUPP); > > - err = (*ifc->ifc_destroy)(ifp); > - if (err != 0) > - return (err); > + (*ifc->ifc_destroy)(ifp); > > /* > * Compute offset in the bitmap and deallocate the unit. > @@ -734,8 +735,15 @@ void > if_clone_attach(ifc) > struct if_clone *ifc; > { > + int bytoff, bitoff; > + int err; > int len, maxclone; > + int unit; > > + KASSERT(ifc->ifc_minifs - 1 <= ifc->ifc_maxunit, > + ("%s: %s requested more units then allowed (%d > %d)", > + __func__, ifc->ifc_name, ifc->ifc_minifs, > + ifc->ifc_maxunit + 1)); > /* > * Compute bitmap size and allocate it. > */ > @@ -745,8 +753,21 @@ if_clone_attach(ifc) > len++; > ifc->ifc_units = malloc(len, M_CLONE, M_WAITOK | M_ZERO); > ifc->ifc_bmlen = len; > + > LIST_INSERT_HEAD(&if_cloners, ifc, ifc_list); > if_cloners_count++; > + > + for (unit = 0; unit < ifc->ifc_minifs; unit++) { > + err = (*ifc->ifc_create)(ifc, unit); > + KASSERT(err == 0, > + ("%s: failed to create required interface %s%d", > + __func__, ifc->ifc_name, unit)); > + > + /* Allocate the unit in the bitmap. */ > + bytoff = unit >> 3; > + bitoff = unit - (bytoff << 3); > + ifc->ifc_units[bytoff] |= (1 << bitoff); > + } > } > > /* > Index: if.h > =================================================================== > RCS file: /usr/cvs/src/sys/net/if.h,v > retrieving revision 1.71 > diff -u -p -r1.71 if.h > --- if.h 19 Mar 2002 21:54:16 -0000 1.71 > +++ if.h 6 Apr 2002 05:41:12 -0000 > @@ -64,16 +64,17 @@ struct if_clone { > LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */ > const char *ifc_name; /* name of device, e.g. `gif' */ > size_t ifc_namelen; /* length of name */ > + int ifc_minifs; /* minimum number of interfaces */ > int ifc_maxunit; /* maximum unit number */ > unsigned char *ifc_units; /* bitmap to handle units */ > int ifc_bmlen; /* bitmap length */ > > int (*ifc_create)(struct if_clone *, int); > - int (*ifc_destroy)(struct ifnet *); > + void (*ifc_destroy)(struct ifnet *); > }; > > -#define IF_CLONE_INITIALIZER(name, create, destroy, maxunit) \ > - { { 0 }, name, sizeof(name) - 1, maxunit, NULL, 0, create, destroy } > +#define IF_CLONE_INITIALIZER(name, create, destroy, minifs, maxunit) \ > + { { 0 }, name, sizeof(name) - 1, minifs, maxunit, NULL, 0, create, destroy } > > /* > * Structure used to query names of interface cloners. > Index: if_disc.c > =================================================================== > RCS file: /usr/cvs/src/sys/net/if_disc.c,v > retrieving revision 1.30 > diff -u -p -r1.30 if_disc.c > --- if_disc.c 14 Dec 2001 19:27:33 -0000 1.30 > +++ if_disc.c 6 Apr 2002 06:51:56 -0000 > @@ -42,6 +42,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -61,20 +62,39 @@ > #define DSMTU 65532 > #endif > > -static void discattach(void); > +#define DISCNAME "disc" > > -static struct ifnet discif; > -static int discoutput(struct ifnet *, struct mbuf *, > - struct sockaddr *, struct rtentry *); > -static void discrtrequest(int, struct rtentry *, struct rt_addrinfo *); > -static int discioctl(struct ifnet *, u_long, caddr_t); > +struct disc_softc { > + struct ifnet sc_if; /* must be first */ > + LIST_ENTRY(disc_softc) sc_list; > +}; > + > +static int discoutput(struct ifnet *, struct mbuf *, > + struct sockaddr *, struct rtentry *); > +static void discrtrequest(int, struct rtentry *, struct rt_addrinfo *); > +static int discioctl(struct ifnet *, u_long, caddr_t); > +static int disc_clone_create(struct if_clone *, int); > +static void disc_clone_destroy(struct ifnet *); > + > +static MALLOC_DEFINE(M_DISC, DISCNAME, "Discard interface"); > +static LIST_HEAD(, disc_softc) disc_softc_list; > +static struct if_clone disc_cloner = IF_CLONE_INITIALIZER(DISCNAME, > + disc_clone_create, disc_clone_destroy, 0, IF_MAXUNIT); > > -static void > -discattach(void) > +static int > +disc_clone_create(struct if_clone *ifc, int unit) > { > - struct ifnet *ifp = &discif; > + struct ifnet *ifp; > + struct disc_softc *sc; > + > + sc = malloc(sizeof(struct disc_softc), M_DISC, M_WAITOK); > + bzero(sc, sizeof(struct disc_softc)); > + > + ifp = &sc->sc_if; > > - ifp->if_name = "ds"; > + ifp->if_softc = sc; > + ifp->if_name = DISCNAME; > + ifp->if_unit = unit; > ifp->if_mtu = DSMTU; > ifp->if_flags = IFF_LOOPBACK | IFF_MULTICAST; > ifp->if_ioctl = discioctl; > @@ -85,6 +105,23 @@ discattach(void) > ifp->if_snd.ifq_maxlen = 20; > if_attach(ifp); > bpfattach(ifp, DLT_NULL, sizeof(u_int)); > + LIST_INSERT_HEAD(&disc_softc_list, sc, sc_list); > + > + return (0); > +} > + > +static void > +disc_clone_destroy(struct ifnet *ifp) > +{ > + struct disc_softc *sc; > + > + sc = ifp->if_softc; > + > + LIST_REMOVE(sc, sc_list); > + bpfdetach(ifp); > + if_detach(ifp); > + > + free(sc, M_DISC); > } > > static int > @@ -92,11 +129,16 @@ disc_modevent(module_t mod, int type, vo > { > switch (type) { > case MOD_LOAD: > - discattach(); > + LIST_INIT(&disc_softc_list); > + if_clone_attach(&disc_cloner); > break; > case MOD_UNLOAD: > - printf("if_disc module unload - not possible for this module type\n"); > - return EINVAL; > + if_clone_detach(&disc_cloner); > + > + while (!LIST_EMPTY(&disc_softc_list)) > + disc_clone_destroy( > + &LIST_FIRST(&disc_softc_list)->sc_if); > + break; > } > return 0; > } > @@ -123,7 +165,7 @@ discoutput(struct ifnet *ifp, struct mbu > m->m_data += sizeof(int); > } > > - if (discif.if_bpf) { > + if (ifp->if_bpf) { > /* > * We need to prepend the address family as > * a four byte field. Cons up a dummy header > @@ -138,7 +180,7 @@ discoutput(struct ifnet *ifp, struct mbu > m0.m_len = 4; > m0.m_data = (char *)⁡ > > - bpf_mtap(&discif, &m0); > + bpf_mtap(ifp, &m0); > } > m->m_pkthdr.rcvif = ifp; > > Index: if_faith.c > =================================================================== > RCS file: /usr/cvs/src/sys/net/if_faith.c,v > retrieving revision 1.14 > diff -u -p -r1.14 if_faith.c > --- if_faith.c 19 Mar 2002 21:54:16 -0000 1.14 > +++ if_faith.c 6 Apr 2002 05:41:12 -0000 > @@ -103,10 +103,10 @@ static MALLOC_DEFINE(M_FAITH, FAITHNAME, > static LIST_HEAD(, faith_softc) faith_softc_list; > > int faith_clone_create(struct if_clone *, int); > -int faith_clone_destroy(struct ifnet *); > +void faith_clone_destroy(struct ifnet *); > > struct if_clone faith_cloner = IF_CLONE_INITIALIZER(FAITHNAME, > - faith_clone_create, faith_clone_destroy, IF_MAXUNIT); > + faith_clone_create, faith_clone_destroy, 0, IF_MAXUNIT); > > #define FAITHMTU 1500 > > @@ -181,7 +181,7 @@ faith_clone_create(ifc, unit) > return (0); > } > > -int > +void > faith_clone_destroy(ifp) > struct ifnet *ifp; > { > @@ -192,7 +192,6 @@ faith_clone_destroy(ifp) > if_detach(ifp); > > free(sc, M_FAITH); > - return (0); > } > > int > Index: if_gif.c > =================================================================== > RCS file: /usr/cvs/src/sys/net/if_gif.c,v > retrieving revision 1.22 > diff -u -p -r1.22 if_gif.c > --- if_gif.c 19 Mar 2002 21:54:18 -0000 1.22 > +++ if_gif.c 6 Apr 2002 05:41:12 -0000 > @@ -90,10 +90,10 @@ void (*ng_gif_attach_p)(struct ifnet *if > void (*ng_gif_detach_p)(struct ifnet *ifp); > > int gif_clone_create(struct if_clone *, int); > -int gif_clone_destroy(struct ifnet *); > +void gif_clone_destroy(struct ifnet *); > > struct if_clone gif_cloner = IF_CLONE_INITIALIZER("gif", > - gif_clone_create, gif_clone_destroy, IF_MAXUNIT); > + gif_clone_create, gif_clone_destroy, 0, IF_MAXUNIT); > > static int gifmodevent(module_t, int, void *); > void gif_delete_tunnel(struct gif_softc *); > @@ -207,7 +207,7 @@ gif_clone_create(ifc, unit) > return (0); > } > > -int > +void > gif_clone_destroy(ifp) > struct ifnet *ifp; > { > @@ -231,7 +231,6 @@ gif_clone_destroy(ifp) > if_detach(ifp); > > free(sc, M_GIF); > - return (0); > } > > static int > Index: if_loop.c > =================================================================== > RCS file: /usr/cvs/src/sys/net/if_loop.c,v > retrieving revision 1.70 > diff -u -p -r1.70 if_loop.c > --- if_loop.c 4 Apr 2002 06:03:17 -0000 1.70 > +++ if_loop.c 6 Apr 2002 05:54:06 -0000 > @@ -110,7 +110,7 @@ static void lortrequest(int, struct rten > int looutput(struct ifnet *ifp, struct mbuf *m, > struct sockaddr *dst, struct rtentry *rt); > int lo_clone_create(struct if_clone *, int); > -int lo_clone_destroy(struct ifnet *); > +void lo_clone_destroy(struct ifnet *); > > struct ifnet *loif = NULL; /* Used externally */ > > @@ -118,10 +118,10 @@ static MALLOC_DEFINE(M_LO, LONAME, "Loop > > static LIST_HEAD(lo_list, lo_softc) lo_list; > > -struct if_clone lo_cloner = > - IF_CLONE_INITIALIZER(LONAME, lo_clone_create, lo_clone_destroy, IF_MAXUNIT); > +struct if_clone lo_cloner = IF_CLONE_INITIALIZER(LONAME, > + lo_clone_create, lo_clone_destroy, 1, IF_MAXUNIT); > > -int > +void > lo_clone_destroy(ifp) > struct ifnet *ifp; > { > @@ -129,17 +129,13 @@ lo_clone_destroy(ifp) > > sc = ifp->if_softc; > > - /* > - * Prevent lo0 from being destroyed. > - */ > - if (loif == ifp) > - return (EINVAL); > + /* XXX: destroying lo0 will lead to panics. */ > + KASSERT(loif != ifp, ("%s: destroying lo0", __func__)); > > bpfdetach(ifp); > if_detach(ifp); > LIST_REMOVE(sc, sc_next); > free(sc, M_LO); > - return (0); > } > > int > @@ -172,16 +168,10 @@ lo_clone_create(ifc, unit) > static int > loop_modevent(module_t mod, int type, void *data) > { > - int err; > - > switch (type) { > case MOD_LOAD: > LIST_INIT(&lo_list); > if_clone_attach(&lo_cloner); > - > - /* Create lo0 */ > - err = if_clone_create("lo0", sizeof ("lo0")); > - KASSERT(err == 0, ("%s: can't create lo0", __func__)); > break; > case MOD_UNLOAD: > printf("loop module unload - not possible for this module type\n"); > Index: if_stf.c > =================================================================== > RCS file: /usr/cvs/src/sys/net/if_stf.c,v > retrieving revision 1.20 > diff -u -p -r1.20 if_stf.c > --- if_stf.c 19 Mar 2002 21:54:18 -0000 1.20 > +++ if_stf.c 6 Apr 2002 05:41:13 -0000 > @@ -158,11 +158,11 @@ static void stf_rtrequest(int, struct rt > static int stf_ioctl(struct ifnet *, u_long, caddr_t); > > int stf_clone_create(struct if_clone *, int); > -int stf_clone_destroy(struct ifnet *); > +void stf_clone_destroy(struct ifnet *); > > /* only one clone is currently allowed */ > struct if_clone stf_cloner = > - IF_CLONE_INITIALIZER(STFNAME, stf_clone_create, stf_clone_destroy, 0); > + IF_CLONE_INITIALIZER(STFNAME, stf_clone_create, stf_clone_destroy, 0, 0); > > int > stf_clone_create(ifc, unit) > @@ -194,7 +194,7 @@ stf_clone_create(ifc, unit) > return (0); > } > > -int > +void > stf_clone_destroy(ifp) > struct ifnet *ifp; > { > @@ -208,7 +208,6 @@ stf_clone_destroy(ifp) > if_detach(ifp); > > free(sc, M_STF); > - return (0); > } > > static int > Index: if_vlan.c > =================================================================== > RCS file: /usr/cvs/src/sys/net/if_vlan.c,v > retrieving revision 1.40 > diff -u -p -r1.40 if_vlan.c > --- if_vlan.c 4 Apr 2002 05:42:09 -0000 1.40 > +++ if_vlan.c 6 Apr 2002 05:41:13 -0000 > @@ -90,7 +90,7 @@ static MALLOC_DEFINE(M_VLAN, "vlan", "80 > static LIST_HEAD(, ifvlan) ifv_list; > > static int vlan_clone_create(struct if_clone *, int); > -static int vlan_clone_destroy(struct ifnet *); > +static void vlan_clone_destroy(struct ifnet *); > static void vlan_start(struct ifnet *ifp); > static void vlan_ifinit(void *foo); > static int vlan_input(struct ether_header *eh, struct mbuf *m); > @@ -102,7 +102,7 @@ static int vlan_unconfig(struct ifnet *i > static int vlan_config(struct ifvlan *ifv, struct ifnet *p); > > struct if_clone vlan_cloner = IF_CLONE_INITIALIZER("vlan", > - vlan_clone_create, vlan_clone_destroy, IF_MAXUNIT); > + vlan_clone_create, vlan_clone_destroy, 0, IF_MAXUNIT); > > /* > * Program our multicast filter. What we're actually doing is > @@ -236,7 +236,7 @@ vlan_clone_create(struct if_clone *ifc, > return (0); > } > > -static int > +static void > vlan_clone_destroy(struct ifnet *ifp) > { > struct ifvlan *ifv = ifp->if_softc; > @@ -250,7 +250,6 @@ vlan_clone_destroy(struct ifnet *ifp) > ether_ifdetach(ifp, ETHER_BPF_SUPPORTED); > > free(ifv, M_VLAN); > - return (0); > } > > static void > > -- > Any statement of the form "X is the one, true Y" is FALSE. > PGP fingerprint 655D 519C 26A7 82E7 2529 9BF0 5D8E 8BE9 F238 1AD4 > To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message