On Aug 12, 2014, at 2:44 PM, Ben Pfaff <[email protected]> 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 <[email protected]>
>
> 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));
> }
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
