On Mon, Sep 12, 2011 at 4:37 PM, Jesse Gross <je...@nicira.com> wrote:
> On Tue, Aug 30, 2011 at 6:59 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 8aec438..3178bdd 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> +static int sample(struct datapath *dp, struct sk_buff *skb,
>> + const struct nlattr *a)
>> +{
>> + u32 probability;
>> + const struct nlattr *sample_args, *nested_act;
>> + int rem, err;
>> +
>> + rem = nla_len(a);
>> +
>> + /* First arg: probability */
>
> It's not really good to be mandating specific positioning of the
> attributes since it makes future compatibility more difficult. This
> is perhaps an exception because it is on the fast path (another good
> reason to not do Netlink parsing here). On the other hand, maybe it's
> better to do it right and then we can optimize it in the future. In
> any case, this function currently has a strange duality between caring
> about performance or not. For example, nla_for_each_nested() has
> extra checks that aren't necessary because we already did validation.
>
I am not sure about future compatibility. Any sampling action must
have probability argument. So how does it make it difficult? do u have
an example of that?
>> + sample_args = nla_data(a);
>> + memcpy(&probability, nla_data(sample_args), sizeof (probability));
>
> I noticed that both in the kernel and userspace there is a lot of
> direct access to the probability argument either like this or treating
> it as unspec. Where possible possible we should use the existing
> helper functions like nla_get_u32().
ok
>
>> + /* Second arg: actions to execute */
>> + sample_args = nla_next(sample_args, &rem);
>> + /* Second arg data contains actions to execute */
>> + nla_for_each_nested(nested_act, sample_args, rem) {
>> + switch (nla_type(nested_act)) {
>> + case OVS_ACTION_ATTR_USERSPACE:
>> + err = output_userspace(dp, skb,
>> + nla_get_u64(nested_act));
>> + return err;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + }
>
> I think that it's reasonable to restrict the supported actions
> initially but it's not clear to me that it's actually a win either
> here or any of the other places that this is now scattered around the
> code base. Since handling the new action list is simply a recursive
> call, I think it's actually pretty straightforward to support all
> actions without any special casing.
>
I can not test arbitrary set of actions that why I restricted that
set. any ways we can allow all
actions except SAMPLE as arg.
>
>> + sample_args = nla_next(sample_args, &rem);
>> + if (nla_type(sample_args) != OVS_SAMPLE_ATTR_ACTIONS)
>> + return -EINVAL;
>> +
>> + /* Second arg data contains actions to execute */
>> + nla_for_each_nested(nested_act, sample_args, rem) {
>> + switch (nla_type(nested_act)) {
>> + case OVS_ACTION_ATTR_USERSPACE:
>> + if (nla_len(nested_act) != 8)
>> + return -EINVAL;
>> + return 0;
>> + break;
>> + default:
>> + /* at this moment userspace is only
>> + * supported action */
>> + return -EINVAL;
>> + }
>> + }
>
> It would be a lot nicer if this function could be replaced with a call
> to nla_validate_nested() and a recursive invocation of
> validate_actions().
ok, if there is no order in arguments and no restrictions on action
set then I can use that.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
