On 8/8/14, 4:46 PM, "Andy Zhou" <az...@nicira.com> wrote:
>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.
Œis_attr_mask_key¹ refers to OVS_FLOW_ATTR_MASK, but I guess that it is
not straightforward. It would be nice to have a more generic netlink
mechanism to solve this issue. Perhaps we should try something like you
suggested (key_len = -2).
>
>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/mail
>>>>>man/
>>>>>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z
>>>>>%2BU
>>>>>6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=9WggZ4ZovrrQ5GRI9bcKsMv%2BY8JSaoZB1XE
>>>>>CseA
>>>>>iUS4%3D%0A&s=06b570a825ecaa3421698357c4647851c197cace2bf3cf1b28d6900c0
>>>>>e5c0
>>>>>cc1
>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev@openvswitch.org
>>>>
>>>>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailm
>>>>an/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw
>>>>5z%2BU6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=28Lt0dzZ7w%2BmkbqJsPC0wd0dWShVt1
>>>>xq2qYIEufBl3k%3D%0A&s=58cd2f65a3fb1ee1048def99555bbccb701f782fe43b37a84
>>>>f233ec44bd31f43
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
