[ovs-dev] [PATCH] v3 datapath: More flexible kernel/userspace tunneling attribute.

Jesse Gross jesse at nicira.com
Fri Jan 18 21:28:19 UTC 2013


On Thu, Jan 17, 2013 at 8:06 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index ed69af8..ab978f4 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> +static struct nlattr *reserve_sfa_size(struct sw_flow_actions **sfa, int attr_len)
> +{
> +
> +       struct sw_flow_actions *new;
> +       struct nlattr *a;
> +       int req_size = NLA_ALIGN(attr_len);
> +       int new_size;
> +
> +       if (req_size <= (ksize(*sfa) - (*sfa)->actions_len))

I think directly using ksize here (in all of these locations) isn't
right because it doesn't take into account struct sw_flow_actions at
the beginning, which is part of the allocation.  We need to subtract
that off the total.

> +               goto out;
> +
> +       new_size = ksize(*sfa) * 2;
> +
> +       if (new_size > MAX_ACTIONS_BUFSIZE) {
> +
> +               if ((MAX_ACTIONS_BUFSIZE - ksize(*sfa)) < req_size)
> +                       return ERR_PTR(-EMSGSIZE);

I think this is a little conservative because it's possible that there
is some space available in the current allocation that when combined
with new space is large enough to fit the new actions.  Therefore, it
would be better to subtract actions_len instead of ksize here.

> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index 5e32965..8e74d1d 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> +enum ovs_tunnel_attr {
> +       OVS_TUNNEL_KEY_ATTR_ID,                 /* be64 Tunnel ID */

I would make the enum name match the constants, like enum ovs_tunnel_key_attr.

> +       OVS_TUNNEL_KEY_ATTR_IPV4_SRC,           /* be32 src IP address. */
> +       OVS_TUNNEL_KEY_ATTR_IPV4_DST,           /* be32 dst IP address. */
> +       OVS_TUNNEL_KEY_ATTR_TOS,                /* u8 Tunnel IP ToS. */
> +       OVS_TUNNEL_KEY_ATTR_TTL,                /* u8 Tunnel IP TTL. */
> +       OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT,      /* No argument, set DF. */
> +       OVS_TUNNEL_KEY_ATTR_CSUM,               /* No argument. CSUM packet. */
> +       __OVS_TUNNEL_KEY_ATTR_MAX
> +};
> +#define OVS_TUNNEL_KEY_ATTR_MAX (__OVS_TUNNEL_KEY_ATTR_MAX - 1)

Most of the other definitions have a blank line between the end of the
enum of the #define.

> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index e2f21da..2be83da 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> +static enum odp_key_fitness
> +tun_key_from_attr(const struct nlattr *attr, struct flow_tnl *tun)
> +{
> +    unsigned int left;
> +    const struct nlattr *a;
> +    bool ttl = false;
> +    bool unknown = false;
> +
> +    NL_NESTED_FOR_EACH(a, left, attr) {
> +        uint16_t type = nl_attr_type(a);
> +        size_t len = nl_attr_get_size(a);
> +        int expected_len = tunnel_key_attr_len(type);
> +
> +        if (len != expected_len && expected_len >= 0) {
> +            return ODP_FIT_ERROR;
> +        }
> +
> +        switch (type) {
> +        case OVS_TUNNEL_KEY_ATTR_ID:
> +            tun->tun_id = nl_attr_get_be64(a);
> +            tun->flags |= FLOW_TNL_F_KEY;
> +        break;
> +        case OVS_TUNNEL_KEY_ATTR_IPV4_SRC:
> +            tun->ip_src = nl_attr_get_be32(a);
> +        break;

I think the userspace coding style is to indent the break to the same
level as the code inside the case (actually I see the same thing for
all the new switch statements both in userspace and kernel).



More information about the dev mailing list