Date: Sat, 6 Apr 2002 16:44:32 -0800 (PST) From: Julian Elischer <julian@elischer.org> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: net@freebsd.org Subject: Re: review request: minor cloning API change Message-ID: <Pine.BSF.4.21.0204061639400.43363-100000@InterJet.elischer.org> In-Reply-To: <20020405230719.A13516@Odin.AC.HMC.Edu>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/param.h>
> #include <sys/systm.h>
> #include <sys/kernel.h>
> +#include <sys/malloc.h>
> #include <sys/module.h>
> #include <sys/mbuf.h>
> #include <sys/socket.h>
> @@ -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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0204061639400.43363-100000>