I agree with Daniele's comments.
Acked-by: Andy Zhou <az...@nicira.com>
mask_set_nlattr() seems to have a single user. may be we cna remove
this function and
call set_nlattr() directly?
Not part of this patch, but set_nlattr() does not look correct in
handling nested attributes.
On Fri, Aug 8, 2014 at 9:25 AM, Daniele Di Proietto
<ddiproie...@vmware.com> wrote:
> Hi Pravin,
>
> couple of comments:
>
> - I would remove also the check for match->mask in update_range__().
> - I think braces are not necessary for if/else statements in the macros.
>
> Of course I would wait for review by Jesse or Andy, but other than that,
> LGTM.
>
> Acked-by: Daniele Di Proietto <ddiproie...@vmware.com>
>
>
> On 8/7/14, 9:26 PM, "Pravin B Shelar" <pshe...@nicira.com> wrote:
>
>>OVS does mask validation even if it does not need to convert
>>netlink mask attributes to mask structure. ovs_nla_get_match()
>>caller can pass NULL mask structure pointer if the caller does
>>not need mask. Therefore NULL check is required in SW_FLOW_KEY*
>>macros. Following patch does not convert mask netlink attributes
>>if mask pointer is NULL, so we do not need these checks in
>>SW_FLOW_KEY* macro.
>>
>>Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
>>---
>> datapath/flow_netlink.c | 74
>>++++++++++++++++++++++++++-----------------------
>> 1 file changed, 39 insertions(+), 35 deletions(-)
>>
>>diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>>index 75172de..63ab7a6 100644
>>--- a/datapath/flow_netlink.c
>>+++ b/datapath/flow_netlink.c
>>@@ -84,8 +84,7 @@ static void update_range__(struct sw_flow_match *match,
>> update_range__(match, offsetof(struct sw_flow_key, field), \
>> sizeof((match)->key->field), is_mask); \
>> if (is_mask) { \
>>- if ((match)->mask) \
>>- (match)->mask->key.field = value; \
>>+ (match)->mask->key.field = value; \
>> } else { \
>> (match)->key->field = value; \
>> } \
>>@@ -95,10 +94,9 @@ static void update_range__(struct sw_flow_match *match,
>> do { \
>> update_range__(match, offset, len, is_mask); \
>> if (is_mask) { \
>>- if ((match)->mask) \
>>- memcpy((u8 *)&(match)->mask->key + offset,
>>value_p, len);\
>>+ memcpy((u8 *)&(match)->mask->key + offset, value_p,
>>len);\
>> } else { \
>>- memcpy((u8 *)(match)->key + offset, value_p, len);
>> \
>>+ memcpy((u8 *)(match)->key + offset, value_p, len); \
>> } \
>> } while (0)
>>
>>@@ -111,9 +109,8 @@ static void update_range__(struct sw_flow_match
>>*match,
>> update_range__(match, offsetof(struct sw_flow_key, field), \
>> sizeof((match)->key->field), is_mask); \
>> if (is_mask) { \
>>- if ((match)->mask) \
>>- memset((u8 *)&(match)->mask->key.field, value,\
>>- sizeof((match)->mask->key.field)); \
>>+ memset((u8 *)&(match)->mask->key.field, value,\
>>+ sizeof((match)->mask->key.field)); \
>> } else { \
>> memset((u8 *)&(match)->key->field, value, \
>> sizeof((match)->key->field)); \
>>@@ -633,8 +630,7 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
>>*match, u64 attrs,
>>
>> SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
>> attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
>>- } else if (!is_mask)
>>- SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>>+ }
>>
>> if (attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
>> __be16 eth_type;
>>@@ -859,8 +855,8 @@ static void mask_set_nlattr(struct nlattr *attr, u8
>>val)
>> * attribute specifies the mask field of the wildcarded flow.
>> */
>> int ovs_nla_get_match(struct sw_flow_match *match,
>>- const struct nlattr *key,
>>- const struct nlattr *mask)
>>+ const struct nlattr *nla_key,
>>+ const struct nlattr *nla_mask)
>> {
>> const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
>> const struct nlattr *encap;
>>@@ -870,7 +866,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>> bool encap_valid = false;
>> int err;
>>
>>- err = parse_flow_nlattrs(key, a, &key_attrs);
>>+ err = parse_flow_nlattrs(nla_key, a, &key_attrs);
>> if (err)
>> return err;
>>
>>@@ -911,35 +907,43 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>> if (err)
>> return err;
>>
>>- if (match->mask && !mask) {
>>- /* Create an exact match mask. We need to set to 0xff all the
>>- * 'match->mask' fields that have been touched in 'match->key'.
>>- * We cannot simply memset 'match->mask', because padding bytes
>>- * and fields not specified in 'match->key' should be left to
>>0.
>>- * Instead, we use a stream of netlink attributes, copied from
>>- * 'key' and set to 0xff: ovs_key_from_nlattrs() will take care
>>- * of filling 'match->mask' appropriately.
>>- */
>>- newmask = kmemdup(key, nla_total_size(nla_len(key)),
>>- GFP_KERNEL);
>>- if (!newmask)
>>- return -ENOMEM;
>>+ if (match->mask) {
>>+
>>+ if (!nla_mask) {
>>+ /* Create an exact match mask. We need to set to 0xff
>>+ * all the 'match->mask' fields that have been touched
>>+ * in 'match->key'. We cannot simply memset
>>+ * 'match->mask', because padding bytes and fields not
>>+ * specified in 'match->key' should be left to 0.
>>+ * Instead, we use a stream of netlink attributes,
>>+ * copied from 'key' and set to 0xff:
>>ovs_key_from_nlattrs()
>>+ * will take care of filling 'match->mask'
>>appropriately.
>>+ */
>>+ newmask = kmemdup(nla_key,
>>+ nla_total_size(nla_len(nla_key)),
>>+ GFP_KERNEL);
>>+ if (!newmask)
>>+ return -ENOMEM;
>>
>>- mask_set_nlattr(newmask, 0xff);
>>+ mask_set_nlattr(newmask, 0xff);
>>
>>- /* The userspace does not send tunnel attributes that are 0,
>>- * but we should not wildcard them nonetheless. */
>>- if (match->key->tun_key.ipv4_dst)
>>- SW_FLOW_KEY_MEMSET_FIELD(match, tun_key, 0xff, true);
>>+ /* The userspace does not send tunnel attributes that
>>+ * are 0, but we should not wildcard them nonetheless.
>>+ */
>>+ if (match->key->tun_key.ipv4_dst)
>>+ SW_FLOW_KEY_MEMSET_FIELD(match, tun_key,
>>+ 0xff, true);
>>
>>- mask = newmask;
>>- }
>>+ nla_mask = newmask;
>>+ }
>>
>>- if (mask) {
>>- err = parse_flow_mask_nlattrs(mask, a, &mask_attrs);
>>+ err = parse_flow_mask_nlattrs(nla_mask, a, &mask_attrs);
>> if (err)
>> goto free_newmask;
>>
>>+ /* Always match on tci. */
>>+ SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>>+
>> if (mask_attrs & 1ULL << OVS_KEY_ATTR_ENCAP) {
>> __be16 eth_type = 0;
>> __be16 tci = 0;
>>--
>>1.9.3
>>
>>_______________________________________________
>>dev mailing list
>>dev@openvswitch.org
>>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z%2BU
>>6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=9WggZ4ZovrrQ5GRI9bcKsMv%2BY8JSaoZB1XECseA
>>iUS4%3D%0A&s=06b570a825ecaa3421698357c4647851c197cace2bf3cf1b28d6900c0e5c0
>>cc1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
