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.
> 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:
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 2a078f3..15e3f18 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1216,7 +1216,14 @@ dpif_netdev_flow_get(const struct dpif *dpif,
}
if (maskp || actionsp) {
- *bufp = ofpbuf_new(1024);
+ struct dp_netdev_actions *actions;
+ size_t len = 0;
+
+ actions = dp_netdev_flow_get_actions(netdev_flow);
+ len += maskp ? sizeof(struct odputil_keybuf) : 0;
+ len += actionsp ? actions->size : 0;
+
+ *bufp = ofpbuf_new(len);
if (maskp) {
struct flow_wildcards wc;
@@ -1231,15 +1238,12 @@ dpif_netdev_flow_get(const struct dpif *dpif,
struct dp_netdev_actions *actions;
actions = dp_netdev_flow_get_actions(netdev_flow);
- *actionsp = ofpbuf_push(*bufp, actions->actions,
actions->size);
+ *actionsp = ofpbuf_put(*bufp, actions->actions,
actions->size);
*actions_len = actions->size;
}
(I plan to repost, but wanted to run this past you)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
