On Fri, Aug 8, 2014 at 1:56 PM, Andy Zhou <az...@nicira.com> wrote:
> I agree with Daniele's comments.
>
> Acked-by: Andy Zhou <az...@nicira.com>
>
Thanks I pushed patch to master.
> 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.
>
can you elaborate on that? It should handle ovs key 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
