On Wednesday 14 August 2013 22:43:05 Mikolaj Golub wrote: > On Wed, Aug 14, 2013 at 05:28:31PM +0200, Marko Zec wrote: > > On Sunday 11 August 2013 22:01:12 Mikolaj Golub wrote: > > > Hi, > > > > > > I would like to commit this patch that fixes some issues related to > > > ipfw nat module load/unload on VIMAGE featured system. > > > > > > Any comments, objections? > > > > Far from being an expert in ipfw, I'm worried that the proposed > > approach of simultaneously acquiring locks on _all_ ipfw instances > > might be calling for trouble: > > > > + VNET_LIST_RLOCK(); > > + VNET_FOREACH(vnet_iter) { > > + CURVNET_SET(vnet_iter); > > + IPFW_WLOCK(&V_layer3_chain); > > + CURVNET_RESTORE(); > > + } > > ipfw_nat_ptr = ipfw_nat; > > lookup_nat_ptr = lookup_nat; > > ipfw_nat_cfg_ptr = ipfw_nat_cfg; > > ipfw_nat_del_ptr = ipfw_nat_del; > > ipfw_nat_get_cfg_ptr = ipfw_nat_get_cfg; > > ipfw_nat_get_log_ptr = ipfw_nat_get_log; > > - IPFW_WUNLOCK(&V_layer3_chain); > > - V_ifaddr_event_tag = EVENTHANDLER_REGISTER( > > + VNET_FOREACH(vnet_iter) { > > + CURVNET_SET(vnet_iter); > > + IPFW_WUNLOCK(&V_layer3_chain); > > + CURVNET_RESTORE(); > > + } > > + VNET_LIST_RUNLOCK(); > > > > Why couldn't we introduce a per-vnet flag, say V_ipfw_nat_ready, and > > use it as > > > > #define IPFW_NAT_LOADED (V_ipfw_nat_ready) > > > > instead of current version of that macro: > > > > #define IPFW_NAT_LOADED (ipfw_nat_ptr != NULL) > > > > I.e., perhaps in ipfw_nat_init() we could first set all the function > > pointers, and then iterate over all vnets and set V_ipfw_nat ready > > there. In ipfw_nat_destroy() we would first iterate over all vnets to > > clear the flag, before clearing function pointers? > > I like you approach. Though insted of iterating vnets in > ipfw_nat_init/destroy I think it is safe just to set/unset > V_ipfw_nat_ready in vnet_ipfw_nat_init/uninit.
Yup that should be safe, provided you're 100% sure that vnet_ipfw_nat_uninit() fires before ipfw_nat_destroy() - admittedly my sysinit ordering insight got a bit rusty so I can't tell at the first glance... Anyhow, this looks fine to me... Marko _______________________________________________ freebsd-virtualization@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-virtualization To unsubscribe, send any mail to "freebsd-virtualization-unsubscr...@freebsd.org"