On 3 July 2014 18:27, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
> On Jul 2, 2014, at 8:22 PM, Joe Stringer <joestrin...@nicira.com> wrote:
>
> 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.
>
>
> We now rely on OVS userspace setting the flag when it needs the reply,
> this in datapath/datapath.c:
>
> /* Check if need to build a reply message.
> * OVS userspace sets the NLM_F_ECHO flag if it needs the reply. */
> static bool ovs_must_notify(struct genl_info *info,
> const struct genl_multicast_group *grp)
> {
> return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
> netlink_has_listeners(genl_info_net(info)->genl_sock, GROUP_ID(grp));
> }
>
> It may be that I had misinterpreted the meaning of the flag, but removing
> it now might not be an option.
>
Specifically for the "flow_get" case, current kernel always skips this
function in ovs_flow_cmd_alloc_info() by passing always=true. I'm not sure
that it makes sense for the kernel to drop the reply if NLM_F_ECHO is not
set, as the point of the operation is to return a particular flow. It would
be more explicit to specify NLM_F_ECHO, but I don't think it's necessary.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
