On Fri, Jul 04, 2014 at 12:15:58PM +1200, Joe Stringer wrote: > Thanks for the review, > > On 4 July 2014 11:53, Ben Pfaff <b...@nicira.com> wrote: > > > On Thu, Jul 03, 2014 at 12:29:25PM +1200, Joe Stringer wrote: > > > Change the interface to allow implementations to pass back a buffer, and > > > allow callers to specify which of actions, mask, and stats they wish to > > > receive. This will be used in the next commit. > > > > > > Signed-off-by: Joe Stringer <joestrin...@nicira.com> > > > > I think that dpif_flow_get() should set *bufp to NULL before it calls > > ->flow_get. Otherwise I think that the error case could end up > > freeing an indeterminate pointer in some cases, e.g. what if > > dpif_netdev_flow_get() returns the error obtained from > > dpif_netdev_flow_from_nlattrs()? > > > > OK, I can do this. I guess this also means that dpif_netdev_flow_get() > doesn't need to set *bufp to NULL on failure.
Yes. > > I think that dpif_netdev_flow_get() has a couple of bugs. First, if > > the actions are big enough that they expand the ofpbuf, and a caller > > asks for the mask also, then adding the actions will invalidate *maskp > > and *mask_len. Second, ofpbuf_put() should be used instead of > > ofpbuf_push(), otherwise the actions will *always* invalidate the mask > > ;-( > > > > I didn't realise before that the "struct dp_netdev_actions" basically holds > the netlink attributes in the form we want them, including the size. > > Is it sufficient to allocate a buffer with sizeof(odputil_keybuf) + > actions->size? ie, the following incremental: That looks like a good approach, yes. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
