On Thu, Aug 31, 2017 at 06:45:16PM +0800, Jiri Benc wrote:
> On Wed, 30 Aug 2017 20:39:12 +0800, Yi Yang wrote:
> > + nsh->md1.context[i] =
> > + OVS_MASKED(nsh->md1.context[i], key->context[i],
> > + mask->context[i]);
> > + }
> > + memcpy(flow_key->nsh.context, nsh->md1.context,
> > + sizeof(nsh->md1.context));
>
> Do you follow the discussion that Hannes Sowa started on the ovs list
> regarding matching on the context fields? It would be better to hold on this
> until there's a conclusion reached.
We had several back-and-forth discussions, Jan was also involved, the
conslusion is current way is still the best way we can have, we need
set and match for context fields.
> > + type,
> > + nla_len(a),
> > + ovs_nsh_key_attr_lens[type].len
> > + );
> > + return -EINVAL;
> > + }
>
> These checks should be done only once when the action is configured, not for
> each packet.
in v8, nsh_key_put_from_nlattr is only one check point, validate_nsh calls it
for set and push_nsh, nsh_hdr_from_nlattr and nsh_key_from_nlattr
haven't these checked anymore because validate_nsh has done them.
> > + if (unlikely(!has_md1 && !has_md2)) {
> > + OVS_NLERR(1, "neither nsh md1 nor md2 attribute is there");
> > + return -EINVAL;
> > + }
>
> Ditto. Plus I don't see a check that the OVS_NSH_KEY_ATTR_BASE attribute is
> present. Seems that the compiler warned you about possibly unitialized flags
> and ttl variables and you just silenced the warning without considering
> whether it's not actually justified.
nsh_key_put_from_nlattr is used for both set and push_nsh, so
OVS_NSH_KEY_ATTR_BASE check is valid only for push_nsh, it is possible
for set not to have OVS_NSH_KEY_ATTR_BASE. v8 has been successfully built
without any warnings, it is also verified in sfc test environment.
I think v8 has fixed all the comments for v7, I have sent out v8 to
mailing list, please help review it, thanks a lot.
