[ovs-dev] [PATCH v2] ofp-actions: Split ofpacts_check__() into many functions.

Yifeng Sun pkusunyifeng at gmail.com
Wed Jul 18 17:13:05 UTC 2018


Sorry I made a mistake in the previous review. Operator '||' has higher
precedence
than operator '?:'. So the patch is correct.

Thanks,
Yifeng

On Tue, Jul 17, 2018 at 1:57 PM, Yifeng Sun <pkusunyifeng at gmail.com> wrote:

> Hi Ben,
>
> I found a small issue:
>
> +{
> +    uint32_t mid = a->meter_id;
> +    return mid == 0 || mid > OFPM13_MAX ? OFPERR_OFPMMFC_INVALID_METER :
> 0;
> +}
>
> If mid == 0 is true, then this function return 1, instead of
> OFPERR_OFPMMFC_INVALID_METER.
> maybe return (mid == 0 || mid > OFPM13_MAX) ?
> OFPERR_OFPMMFC_INVALID_METER : 0;
>
> Other than that, this patch looks good to me.
> 'make check' passed all tests.
>
> Thanks.
>
> Tested-by: Yifeng Sun <pkusunyifeng at gmail.com>
> Reviewed-by: Yifeng Sun <pkusunyifeng at gmail.com>
>
>
> On Thu, Jul 12, 2018 at 4:33 PM, Ben Pfaff <blp at ovn.org> wrote:
>
>> On Fri, Jun 15, 2018 at 04:29:22PM -0700, Ben Pfaff wrote:
>> > ofpacts_check__() was a huge switch statement with special cases for
>> many
>> > different kinds of actions.  This made it unwieldy and put the special
>> > cases far away from the rest of the code related to a given action.
>> This
>> > commit refactors the code to avoid the problem.
>> >
>> > Signed-off-by: Ben Pfaff <blp at ovn.org>
>> > ---
>> > I'm reposting this in hope of getting a review this time.
>>
>> Still needs review.
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>


More information about the dev mailing list