RFC: ipfw nat VIMAGE improvements

[Marko Zec]

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?

Marko