[ovs-dev] [PATCH v4 1/4] lib/odp: Masked set action execution and printing.

Jarno Rajahalme jrajahalme at nicira.com
Mon Aug 25 17:44:12 UTC 2014


On Aug 12, 2014, at 2:44 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Mon, Aug 11, 2014 at 09:14:58AM -0700, Jarno Rajahalme wrote:
>> Add a new action type OVS_ACTION_ATTR_SET_MASKED, and support for
>> parsing, printing, and committing them.
>> 
>> Masked set actions add a mask, immediately following the netlink
>> attribute data, within the netlink attribute itself.  Thus the key
>> attribute size for a masked set action is exactly double of the
>> non-masked set action.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> mask_ipv6_addr() does the kind of aliasing that some GCC versions
> might not like.  (I don't have a convenient copy of GCC 4.1 to try.)
> 

OK, using the ovs_16aligned_be32 like I should have in the first place :-)

> I think that the OVS_ACTION_ATTR_SET_MASKED case in
> format_odp_actions() can be tricked into accessing misaligned memory
> (on RISC), if nl_attr_size(a) is not a multiple of 8.
> 

Don't see this, as the data is memcpy'ed to properly aligned local buffers:

>     case OVS_ACTION_ATTR_SET_MASKED:
>         a = nl_attr_get(a);
>         size = nl_attr_get_size(a) / 2;
>         ds_put_cstr(ds, "set(");
> 
>         /* Masked set action not supported for tunnel key, which is bigger. */
>         if (size <= sizeof(struct ovs_key_ipv6)) {
>             struct nlattr attr[1 + DIV_ROUND_UP(sizeof(struct ovs_key_ipv6),
>                                                 sizeof(struct nlattr))];
>             struct nlattr mask[1 + DIV_ROUND_UP(sizeof(struct ovs_key_ipv6),
>                                                 sizeof(struct nlattr))];
> 
>             mask->nla_type = attr->nla_type = nl_attr_type(a);
>             mask->nla_len = attr->nla_len = NLA_HDRLEN + size;
>             memcpy(attr + 1, (char *)(a + 1), size);
>             memcpy(mask + 1, (char *)(a + 1) + size, size);
>             format_odp_key_attr(attr, mask, NULL, ds, true);
>         } else {
>             format_odp_key_attr(a, NULL, NULL, ds, true);
>         }
>         ds_put_cstr(ds, ")");
>         break;


> parse_odp_action() should use is_all_ones() instead of an open-coded
> loop.
> 

OK.

> Here are some style-only changes that make this code easier for me
> to read:
> 

Fixed. Btw, is there an Emacs setting that I could use to indent function calls like this by pressing the tab-key? Mine wants the parameters to be indented after the opening parenthesis.

> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 098dee9..e1093a2 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -35,12 +35,11 @@
> 
> /* Masked copy of an ethernet address. 'src' is already properly masked. */
> static void
> -ether_addr_copy_masked(uint8_t *dst, const uint8_t *src,
> -                       const uint8_t *mask)
> +ether_addr_copy_masked(uint8_t *dst, const uint8_t *src, const uint8_t *mask)
> {
>     int i;
> 
> -    for (i=0; i < ETH_ADDR_LEN; i++) {
> +    for (i = 0; i < ETH_ADDR_LEN; i++) {
>         dst[i] = src[i] | (dst[i] & ~mask[i]);
>     }
> }
> @@ -68,13 +67,12 @@ odp_set_ipv4(struct ofpbuf *packet, const struct ovs_key_ipv4 *key,
> {
>     struct ip_header *nh = ofpbuf_l3(packet);
> 
> -    packet_set_ipv4(packet,
> -                    key->ipv4_src
> -                    | (get_16aligned_be32(&nh->ip_src) & ~mask->ipv4_src),
> -                    key->ipv4_dst
> -                    | (get_16aligned_be32(&nh->ip_dst) & ~mask->ipv4_dst),
> -                    key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos),
> -                    key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl));
> +    packet_set_ipv4(
> +        packet,
> +        key->ipv4_src | (get_16aligned_be32(&nh->ip_src) & ~mask->ipv4_src),
> +        key->ipv4_dst | (get_16aligned_be32(&nh->ip_dst) & ~mask->ipv4_dst),
> +        key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos),
> +        key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl));
> }
> 
> static const ovs_be32 *
> @@ -101,14 +99,14 @@ odp_set_ipv6(struct ofpbuf *packet, const struct ovs_key_ipv6 *key,
>     uint8_t old_tc = ntohl(get_16aligned_be32(&nh->ip6_flow)) >> 20;
>     ovs_be32 old_fl = get_16aligned_be32(&nh->ip6_flow) & htonl(0xfffff);
> 
> -    packet_set_ipv6(packet, key->ipv6_proto,
> -                    mask_ipv6_addr((const ovs_be16 *)nh->ip6_src.be32,
> -                                   key->ipv6_src, mask->ipv6_src, sbuf),
> -                    mask_ipv6_addr((const ovs_be16 *)nh->ip6_dst.be32,
> -                                   key->ipv6_dst, mask->ipv6_dst, dbuf),
> -                    key->ipv6_tclass | (old_tc & ~mask->ipv6_tclass),
> -                    key->ipv6_label | (old_fl & ~mask->ipv6_label),
> -                    key->ipv6_hlimit | (nh->ip6_hlim & ~mask->ipv6_hlimit));
> +    packet_set_ipv6(
> +        packet,
> +        key->ipv6_proto,
> +        mask_ipv6_addr(nh->ip6_src.be16, key->ipv6_src, mask->ipv6_src, sbuf),
> +        mask_ipv6_addr(nh->ip6_dst.be16, key->ipv6_dst, mask->ipv6_dst, dbuf),
> +        key->ipv6_tclass | (old_tc & ~mask->ipv6_tclass),
> +        key->ipv6_label | (old_fl & ~mask->ipv6_label),
> +        key->ipv6_hlimit | (nh->ip6_hlim & ~mask->ipv6_hlimit));
> }
> 
> static void
> @@ -117,7 +115,8 @@ odp_set_tcp(struct ofpbuf *packet, const struct ovs_key_tcp *key,
> {
>     struct tcp_header *th = ofpbuf_l4(packet);
> 
> -    packet_set_tcp_port(packet, key->tcp_src | (th->tcp_src & ~mask->tcp_src),
> +    packet_set_tcp_port(packet,
> +                        key->tcp_src | (th->tcp_src & ~mask->tcp_src),
>                         key->tcp_dst | (th->tcp_dst & ~mask->tcp_dst));
> }
> 
> @@ -127,7 +126,8 @@ odp_set_udp(struct ofpbuf *packet, const struct ovs_key_udp *key,
> {
>     struct udp_header *uh = ofpbuf_l4(packet);
> 
> -    packet_set_udp_port(packet, key->udp_src | (uh->udp_src & ~mask->udp_src),
> +    packet_set_udp_port(packet,
> +                        key->udp_src | (uh->udp_src & ~mask->udp_src),
>                         key->udp_dst | (uh->udp_dst & ~mask->udp_dst));
> }
> 




More information about the dev mailing list