On Wed, Jan 20, 2016 at 11:31:07AM -0500, Russell Bryant wrote: > Before this patch, physical.c build up the set of local datapaths for > its own use. I'd like to use it in another module in a later patch, so > pull it out of physical. It's now populated by the bindings module, > since that seems like a more appropriate place to do it, and it's also > done much earlier in the main loop, making it easier to re-use. > > Signed-off-by: Russell Bryant <russ...@ovn.org> > Acked-by: Han Zhou <zhou...@gmail.com> > --- > > v1->v2: > - added ack from Han Zhou
I guess I reviewed v1 accidentally. I'll copy it here. The scoping is weird. The hmap is torn down and recreated on each main loop iteration, but instead of constructing it at the beginning of an iteration and tearing it down at the end, it's torn down and then rebuilt at the beginning, which means that after the main loop we have to take another shot at tearing it down again. The method of tearing down the hmap is needlessly inefficient. Instead of iterating through the datapaths and removing them one by one, each time we start over from the beginning and look for the first one. This isn't new but I didn't notice it before: > + /* Clear local_datapaths, as we're going to rebuild it next. */ > + struct hmap_node *node; > + while ((node = hmap_first(local_datapaths))) { > + hmap_remove(local_datapaths, node); > + free(node); > + } I guess the obvious HMAP_FOR_EACH_SAFE isn't being used because there's no outer struct. Maybe an outer struct should be defined, even if it's just a bare wrapper around hmap_node. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev