[ovs-dev] [bug 7759 06/11] datapath: Move Netlink PID for userspace actions from flows to actions.
Jesse Gross
jesse at nicira.com
Wed Oct 12 00:34:08 UTC 2011
On Wed, Oct 5, 2011 at 11:27 AM, Ben Pfaff <blp at nicira.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 15c1e33..10b0a0c 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> +static int validate_userspace(const struct nlattr *attr)
> +{
> + static const struct nla_policy userspace_policy[OVS_USERSPACE_ATTR_MAX + 1] =
> + {
> + [OVS_USERSPACE_ATTR_PID] = {.type = NLA_U32 },
> + [OVS_USERSPACE_ATTR_USERDATA] = {.type = NLA_U64 },
> + };
> + struct nlattr *a[OVS_USERSPACE_ATTR_MAX + 1];
> + int error;
> +
> + error = nla_parse_nested(a, OVS_USERSPACE_ATTR_MAX, attr, userspace_policy);
> + if (error)
> + return error;
> +
> + if (!a[OVS_USERSPACE_ATTR_PID] || !nla_get_u32(a[OVS_USERSPACE_ATTR_PID]))
> + return -EINVAL;
For vports, if there is no PID we default to using the one associated
with the Netlink request. I'm not sure that that is really all that
useful of a feature because it is easy enough for userspace to
explicitly set the PID to itself explicitly if that's what it wants
but it is inconsistent with this. I think it would be somewhat
difficult to have that behavior here, so should we just make PID
mandatory when creating datapaths and vports?
> diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
> index 3db960a..d058899 100644
> --- a/include/openvswitch/datapath-protocol.h
> +++ b/include/openvswitch/datapath-protocol.h
> +/**
> + * enum ovs_userspace_attr - Attributes for %OVS_ACTION_ATTR_USERSPACE action.
> + * @OVS_USERSPACE_ATTR_PID: u32 Netlink PID to which the %OVS_PACKET_CMD_ACTION
> + * message should be sent. Required.
> + * @OVS_USERSPACE_ATTR_USERDATA: If present and nonzero, its u64 argument is
> + * copied to the %OVS_PACKET_CMD_ACTION message as %OVS_PACKET_ATTR_USERDATA,
At the moment, the kernel actually always sends the userdata for
command upcalls - it seems a little nicer to not assume that zero is a
special value. Probably the best thing to do is actually send
userdata if and only if it was actually set up originally.
> diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> index 79e84d6..1395c99 100644
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> +static uint32_t
> +dpif_linux_port_get_pid(const struct dpif *dpif_, uint16_t port_no)
> +{
> + struct dpif_linux *dpif = dpif_linux_cast(dpif_);
> +
> + if (!(dpif->listen_mask & (1u << DPIF_UC_MISS))) {
> + return 0;
Since this is now used for ACTION upcalls as well, I don't think this
mask is quite right.
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 9e8b1fb..f24cafd 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> +static size_t
> +put_userspace_action(const struct ofproto_dpif *ofproto,
> + struct ofpbuf *odp_actions,
> + const struct flow *flow,
> + const struct user_action_cookie *cookie)
> +{
> + size_t offset;
> + uint32_t pid;
> +
> + BUILD_ASSERT_DECL(sizeof *cookie == sizeof(uint64_t));
We already have a BUILD_ASSERT_DECL where struct user_action_cookie is
defined in lib/odp-util.h, is there a reason to have another here?
More information about the dev
mailing list