Thanks for the corrections. I sent out a v2 with those fixes.
I'd really like to get multiple reviews on this. Justin, are you
willing to spend some time looking over v2?
Thanks,
Ben.
On Wed, Apr 24, 2013 at 09:44:20PM +0100, Zoltan Kiss wrote:
> 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
