[ovs-dev] [PATCHv2 5/6] ofproto-provider: Add action validation.

[Jarno Rajahalme]

I would urge Ben to check this up as well, though.

  Jarno

> On Nov 11, 2015, at 2:23 PM, Jarno Rajahalme <jarno at ovn.org> wrote:
> 
> With a comment below,
> 
> Acked-by: Jarno Rajahalme <jarno at ovn.org>
> 
>> On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestringer at nicira.com> wrote:
>> 
>> Add an ofproto-level function to allow implementations to reject
>> specific action types based on internal implementation details. The
>> first user will be the next patch, which checks for datapath (kernel)
>> support for various aspects of connection tracking and uses this to
>> allow or reject ct() actions.
>> 
>> Signed-off-by: Joe Stringer <joestringer at nicira.com>
>> CC: Jarno Rajahalme <jarno at ovn.org>
>> ---
>> ofproto/ofproto-dpif.c     |  1 +
>> ofproto/ofproto-provider.h | 14 ++++++++++++++
>> ofproto/ofproto.c          |  9 +++++++++
>> 3 files changed, 24 insertions(+)
>> 
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index f0a2ca59e2e8..8b1760c95409 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -5689,6 +5689,7 @@ const struct ofproto_class ofproto_dpif_class = {
>>    port_poll_wait,
>>    port_is_lacp_current,
>>    port_get_lacp_stats,
>> +    NULL,
>>    NULL,                       /* rule_choose_table */
>>    rule_alloc,
>>    rule_construct,
>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
>> index 117cd1fcc02e..da6cb37c0b60 100644
>> --- a/ofproto/ofproto-provider.h
>> +++ b/ofproto/ofproto-provider.h
>> @@ -1077,6 +1077,20 @@ struct ofproto_class {
>> /* ## OpenFlow Rule Functions ## */
>> /* ## ----------------------- ## */
>> 
>> +    /* Checks whether the action 'ofpact' is supported by 'ofproto'. On
>> +     * success, returns 0. On failure, returns an OpenFlow error code.
>> +     *
>> +     * Some actions are marked as optional by the OpenFlow specification.
>> +     * Furthermore, OVS includes support for several vendor extensions which
>> +     * may not be supported by all ofproto implementations. This function
>> +     * allows specific actions to be rejected based on internal datapath
>> +     * support. Failure implies that an OpenFlow rule that includes 'ofpact'
>> +     * in its actions can never be inserted into 'ofproto'.
>> +     *
>> +     * If this function is NULL then all actions are supported. */
>> +    enum ofperr (*rule_check_action)(const struct ofproto *ofproto,
>> +                                     const struct ofpact *ofpact);
>> +
>>    /* Chooses an appropriate table for 'match' within 'ofproto'.  On
>>     * success, stores the table ID into '*table_idp' and returns 0.  On
>>     * failure, returns an OpenFlow error code.
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index c7dd8a2c991b..338330108df1 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -3346,10 +3346,19 @@ ofproto_check_ofpacts(struct ofproto *ofproto,
>>    }
>> 
>>    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
>> +        enum ofperr error;
>> +
>>        if (a->type == OFPACT_GROUP
>>            && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id)) {
>>            return OFPERR_OFPBAC_BAD_OUT_GROUP;
>>        }
>> +
>> +        if (ofproto->ofproto_class->rule_check_action) {
>> +            error = ofproto->ofproto_class->rule_check_action(ofproto, a);
>> +            if (error) {
>> +                return error;
>> +            }
>> +        }
> 
> I would make this call for ‘known to possibly not be supported’ actions, in the spirit of the group check above.
> 
>>    }
>> 
>>    return 0;
>> -- 
>> 2.1.4
>> 
>