On Fri, Aug 8, 2014 at 3:58 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> 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.
The minor issue is that the name of the 3rd argument
"is_attr_mask_key" seems wrong. We first pass in with 'true' then
change to 'false' for nested attribute does not make sense. It is
still part of mask.
The main issue is that this logic does not handle genuine multiple
level of nested attributes. For example, encap within tunnel. OVS
does not support ittoday but Geneve does allow for outer header to
have a vlan tag.
If the issue is to handle variable length key types, such as GENEVE
options, we should try to tell that apart from the nested ones, may be
using key_len of -2?
Make sense?
>
>>
>> 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
