[ovs-dev] [PATCH v3 1/6] Add a new OVS action check_pkt_larger

[Gregory Rose]

On 4/18/2019 2:48 PM, Ben Pfaff wrote:
> On Tue, Apr 16, 2019 at 01:23:37PM +0530, nusiddiq at redhat.com wrote:
>> From: Numan Siddique <nusiddiq at redhat.com>
>>
>> This patch adds a new action 'check_pkt_larger' which checks if the
>> packet is larger than the given size and stores the result in the
>> destination register.
> Thanks for the revision!  I'm taking a detailed look at this series now
> that the datapath action is upstream and reviewed for backport.
>
> (In my opinion, it would be better if the datapath backport was patch 1,
> but it is not a big deal.)

FWIW patches 1 and 2 of the series apply well independently or 
together.  All of my testing was done
with both patches 1 and 2 applied.  However, I did not do a full review 
of all the code in the first patch of the
series due to time constraints.  Patch 1 of the series did apply cleanly 
and passed checkpatch.

- Greg

>
> odp_execute_check_pkt_len() checks the action pretty carefully.  It
> might not be necessary, since the action should be generated by code
> elsewhere in the same process.  If you decide that you like the checks
> anyway (I do not object), then:
>
>      * NL_NESTED_FOR_EACH is safer than NL_NESTED_FOR_EACH_UNSAFE, so you
>        might consider it.
>
>      * ovs_assert() is more usual than if (...) { OVS_NOT_REACHED(); }
>
> odp_execute_check_pkt_len() could use nl_parse_nested(), like
> format_odp_check_pkt_len_action() does.  nl_parse_nested() does thorough
> error checking internally.  The only reason the odp-execute.c functions
> tend not to use nl_parse_nested() and friends is because they trust the
> action.  If you prefer not to trust it, nl_parse_nested() might be
> simpler.
>
> There's a missing blank line here:
>
> @@ -353,10 +353,12 @@ enum ofp_raw_action_type {
>       NXAST_RAW_DECAP,
>   
>       /* NX1.3+(48): void. */
>       NXAST_RAW_DEC_NSH_TTL,
>   
> +    /* NX1.0+(49): struct nx_action_check_pkt_larger, ... VLMFF */
> +    NXAST_RAW_CHECK_PKT_LARGER,
>   /* ## ------------------ ## */
>   /* ## Debugging actions. ## */
>   /* ## ------------------ ## */
>   
>   /* These are intentionally undocumented, subject to change, and ovs-vswitchd */
>
> struct nx_action_check_pkt_larger encodes a number of bits, but only a
> single bit is allowed.  Thus, the action structure shouldn't have a
> space for a count of bits.
>
> Somewhere, we need to document how the packet's size is calculated.
> Probably this should be in lib/ovs-actions.xml.  The example there
> implies that the argument includes the L2 payload but not the L2 header,
> so that a maximum-length Ethernet packet is 1500 bytes (not 1514).  If
> there are any other special cases or exceptions (for example, does a
> VLAN header count against the size limit?), then it would be good to
> describe them there too.
>
> Thanks,
>
> Ben.