Re: correct way to pass callbacks

On 2007-02-25 21:37, Andrew Thompson <thompsa_at_freebsd.org> wrote:
> Hi,
> The bridgestp module needs two callbacks from the bridge when it
> attaches which so far I have just passed on with the function call.
> 
> bstp_attach(&sc->sc_stp, bridge_state_change, bridge_rtable_expire);
> 
> I have always felt this was rather ugly so have attached a patch to put
> them both in a struct, is this the right way to do it?

This is one of the good ways to do it, AFAIK.  It's similar to the way
vnode operations are 'abstracted' away in a vop_vector struct.

Converting two functino arguments to a set of callbacks inside a struct
is also going to be more extensible in the future, when new callbacks of
'bridge ops' are required.  Then we can add something like:

    struct bridge_ops_vector {
      .bstatechange = bridge_state_change,
      .brtexpire =    bridge_rtable_expire,
      ...
   };

I think I like this :)

> Index: bridgestp.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/net/bridgestp.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 bridgestp.c
> --- bridgestp.c	18 Jan 2007 07:13:01 -0000	1.34
> +++ bridgestp.c	23 Feb 2007 22:27:08 -0000
> _at__at_ -2087,8 +2087,7 _at__at_ DECLARE_MODULE(bridgestp, bstp_mod, SI_S
>  MODULE_VERSION(bridgestp, 1);
>  
>  void
> -bstp_attach(struct bstp_state *bs, bstp_state_cb_t state_callback,
> -    bstp_rtage_cb_t rtage_callback)
> +bstp_attach(struct bstp_state *bs, struct bstp_cb *cb)
>  {
>  	BSTP_LOCK_INIT(bs);
>  	callout_init_mtx(&bs->bs_bstpcallout, &bs->bs_mtx, 0);
> _at__at_ -2102,8 +2101,8 _at__at_ bstp_attach(struct bstp_state *bs, bstp_
>  	bs->bs_migration_delay = BSTP_DEFAULT_MIGRATE_DELAY;
>  	bs->bs_txholdcount = BSTP_DEFAULT_HOLD_COUNT;
>  	bs->bs_protover = BSTP_PROTO_RSTP;
> -	bs->bs_state_cb = state_callback;
> -	bs->bs_rtage_cb = rtage_callback;
> +	bs->bs_state_cb = cb->bcb_state;
> +	bs->bs_rtage_cb = cb->bcb_rtage;
>  
>  	getmicrotime(&bs->bs_last_tc_time);
>  
> Index: bridgestp.h
> ===================================================================
> RCS file: /home/ncvs/src/sys/net/bridgestp.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 bridgestp.h
> --- bridgestp.h	11 Dec 2006 23:46:40 -0000	1.12
> +++ bridgestp.h	23 Feb 2007 22:25:27 -0000
> _at__at_ -186,6 +186,11 _at__at_
>  typedef void (*bstp_state_cb_t)(struct ifnet *, int);
>  typedef void (*bstp_rtage_cb_t)(struct ifnet *, int);
>  
> +struct bstp_cb {
> +	bstp_state_cb_t bcb_state;
> +	bstp_rtage_cb_t bcb_rtage;
> +};
> +
>  /*
>   * Because BPDU's do not make nicely aligned structures, two different
>   * declarations are used: bstp_?bpdu (wire representation, packed) and
> _at__at_ -365,7 +370,7 _at__at_ extern const uint8_t bstp_etheraddr[];
>  
>  extern	void (*bstp_linkstate_p)(struct ifnet *ifp, int state);
>  
> -void	bstp_attach(struct bstp_state *, bstp_state_cb_t, bstp_rtage_cb_t);
> +void	bstp_attach(struct bstp_state *, struct bstp_cb *);
>  void	bstp_detach(struct bstp_state *);
>  void	bstp_init(struct bstp_state *);
>  void	bstp_stop(struct bstp_state *);
> Index: if_bridge.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/net/if_bridge.c,v
> retrieving revision 1.92
> diff -u -p -r1.92 if_bridge.c
> --- if_bridge.c	11 Dec 2006 23:46:40 -0000	1.92
> +++ if_bridge.c	23 Feb 2007 22:26:48 -0000
> _at__at_ -528,6 +528,7 _at__at_ bridge_clone_create(struct if_clone *ifc
>  {
>  	struct bridge_softc *sc, *sc2;
>  	struct ifnet *bifp, *ifp;
> +	struct bstp_cb cb;
>  	u_char eaddr[6];
>  	int retry;
>  
> _at__at_ -583,7 +584,9 _at__at_ bridge_clone_create(struct if_clone *ifc
>  		mtx_unlock(&bridge_list_mtx);
>  	}
>  
> -	bstp_attach(&sc->sc_stp, bridge_state_change, bridge_rtable_expire);
> +	cb.bcb_state = bridge_state_change;
> +	cb.bcb_rtage = bridge_rtable_expire;
> +	bstp_attach(&sc->sc_stp, &cb);
>  	ether_ifattach(ifp, eaddr);
>  	/* Now undo some of the damage... */
>  	ifp->if_baudrate = 0;