I've reviewed and tested this. In the patch description there are one
small mistakes:
-execute_controller_action(), by passing true instead of false as its
+execute_controller_action(), by passing false instead of true as its
But more importantly, todo in handle_miss_upcalls are freed twice, and
used after the first free, which cause segfaults. After modifying the
third chunk it worked:
@@ -2710,14 +2709,10 @@ handle_miss_upcalls(struct ofproto_dpif
/* Process each element in the to-do list, constructing the set of
* operations to batch. */
n_ops = 0;
- HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
+ HMAP_FOR_EACH (miss, hmap_node, &todo) {
handle_flow_miss(ofproto, miss, flow_miss_ops, &n_ops);
- ofpbuf_list_delete(&miss->packets);
- hmap_remove(&todo, &miss->hmap_node);
- free(miss);
}
assert(n_ops <= ARRAY_SIZE(flow_miss_ops));
- hmap_destroy(&todo);
/* Execute batch. */
for (i = 0; i < n_ops; i++) {
Zoli
On 23/04/13 18:16, Ben Pfaff wrote:
One plausible solution seemed to be to turn over ownership of the packet to
execute_controller_action(), by passing true instead of false as its
'clone' argument. Another is to add an else case to the "if" statement
that calls execute_controller_action() to free the packet.
@@ -2710,11 +2709,8 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct
dpif_upcall *upcalls,
/* Process each element in the to-do list, constructing the set of
* operations to batch. */
n_ops = 0;
- HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
+ HMAP_FOR_EACH (miss, hmap_node, &todo) {
handle_flow_miss(ofproto, miss, flow_miss_ops, &n_ops);
- ofpbuf_list_delete(&miss->packets);
- hmap_remove(&todo, &miss->hmap_node);
- free(miss);
}
assert(n_ops <= ARRAY_SIZE(flow_miss_ops));
hmap_destroy(&todo);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
