[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