On Thu, May 14, 2015 at 08:04:02PM -0700, Shashank Shanbhag wrote: > > It's unusual to dynamically allocate an ovs_list head. I would > > ordinarily just declare 'tables' as a plain ovs_list instead of a > > pointer. > > > > Do you think there is much value in maintaining the order of table > > update using the linked list? It seems likely to me that ordinarily > > the update order is not important, and in cases where it is important > > it seems that the user could call ovs-ofctl more than once. It would > > simplify the implementation a little, and I guess that it would be > > just as easy to explain, if the tables were simply updated in order of > > increasing table number. > > I think update order matters. I would expect table 0 to be updated last > after all the flows in later tables are updated to, say, reflect a change > in policy. > > Also, calling ovs-ofctl 255 times in the worst case would be really > expensive, considering the fact that the FILE can be large, and the > connection may be over SSL.
OK, that's fine then. > > I think that we might as well just use an array for the table of > > classifiers. It's only an array of 255 elements, after all. I don't > > see a benefit to using a hash table. > > Trying to avoid the array of 255 elements, especially for cases where there > are less number of tables being replaced. An array of 255 bytes is not a big deal, and it would be easier to understand. > > In read_flows_from_switch(), it seems odd to me to use > > parse_ofp_flow_stats_request_str() to initialize the flow stats > > request. I would tend to continue initializing it "by hand". > > I'd like to avoid doing it this way. Any future updates will need to be > done at a single place. Also, parse_ofp_flow_stats_request_str() is not > being used anywhere in the code, FYI. I must have been looking at something else, the code here look fine to me in v2. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
