[ovs-dev] [PATCHv2 1/2] dpif: Support fetching flow mask via dpif_flow_get().

[Ben Pfaff]

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 <blp at 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 <joestringer at 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.