I appreciate the expedient review. This series doesn't actually depend on the batched flow_get, but this was a starting point/RFC for discussion.
On 3 July 2014 09:24, Ben Pfaff <b...@nicira.com> wrote: > On Wed, Jul 02, 2014 at 08:14:14PM +1200, Joe Stringer wrote: > > This patch adds support for batching flow_get operations to the dpif. As > > part of this, it also now allows fetching the mask (this was previously > > not possible using a 'get' operation). > > > > Signed-off-by: Joe Stringer <joestrin...@nicira.com> > > This approach makes sense. > > The comments on dpif_flow_get() don't match the parameters. (Possibly > we could just remove that function. It has no callers.) > dpif_flow_get() is the main change that this series depends on. I have stripped this patch down to the minimal required for v2 (branch-2.3 only): http://openvswitch.org/pipermail/dev/2014-July/042423.html I think it might be time to make the dpif_class 'operate' function > non-optional. At that point, we can delete 'flow_get', 'flow_put', > 'execute', and 'flow_del' from the dpif_class interface. (This > doesn't have to be done in this patch or this series, it's just a > note.) > This makes sense. As I see it, the individual get/put/del/execute commands are just a special case (n_ops=1) of the 'operate' function. In dpif_linux_operate__(), I think that it is unnecessary to put > NLM_F_ECHO in the message flags. > > I do not think that the code in dpif_linux_operate__() is safe. It > looks to me like the reply is constructed in a stub on the stack (in > the local auxes[] array) and then the caller's get->buffer points into > that stub. If so, then I think it would be better to require the > caller to not just supply a pointer to an ofpbuf but actually an > initialized ofpbuf that includes storage (probably a stub in most > cases) that dpif_operate() can use. > I agree with this assessment. If/when I repost this flow_get operate patch, I will plan to fold in these changes. I don't think the comment on 'buffer' in the definition of > dpif_flow_get() describes the semantics well enough. As is, I think > that any programmer who tries to use the interface will have to read > code a couple of layers deep, into dpif-linux or dpif-netdev, to use > it correctly. > The v2 makes this more explicit for the flow_get interface. I'll plan to take this feedback and any feedback you may have for the v2 interface, and roll that into the equivalent changes for master. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
