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.) 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.) 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 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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
