[ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

Greg Rose gvrose8192 at gmail.com
Fri May 26 15:00:31 UTC 2017


On Fri, 2017-05-26 at 01:53 +0000, Darrell Ball wrote:
> 
> On 5/22/17, 4:44 PM, "ovs-dev-bounces at openvswitch.org on behalf of Joe Stringer" <ovs-dev-bounces at openvswitch.org on behalf of joe at ovn.org> wrote:
> 
>     On 20 May 2017 at 11:09, Darrell Ball <dlu998 at gmail.com> wrote:
>     > The checks to populate ct_orig_tuple in miniflow_extract
>     > includes recirc_id being non-zero.  This is changed here
>     > to populate the ct_orig_tuple fields based only on ct_state
>     > per the logic of the design.
>     >
>     > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
>     
>     I'm not exactly sure what "per the logic of the design" is supposed to
>     mean, and I suspect that someone reading through the git logs would be
>     just as lost. As far as I'm aware, any patch to OVS should be "per the
>     logic of the design", right?
> 
> In general, a change can be consistent with an 
> existing design or a change to the existing design itself. 
> However, “per the logic of the design” can be assumed by default
> and hence is usually considered superfluous, which I think is what you
> want to convey – sure.

At the time I thought you meant more like 'per the specification'.  Now
I realize I came off as an idiot.  So is there a specification for what
the ct_orig_tuple fields should be set to?  Is there some rule we can
refer to that states explicitly that ct_orig_tuple should be populated
based only upon ct_state?  Or is it something that should just be clear
by understanding the code?

Sorry if I'm missing something obvious.

- Greg

> 
> I’ll include the text I used on the related thread, as I agree, the
> motivation is not necessarily obvious, just by linking ct_orig_tuple to
> ct_state, as I did in this commit message.
>     
>     Do you mean "The ct_orig_tuple fields are designed to be read only if
>     the ct_state is valid, so do not populate that field if the packet has
>     not gone through the connection tracker"?
> 
>     I think that this kind of
>     description would provide at least a little extra context to help
>     understand the motivation behind the change.
> 
> 
> 
>     _______________________________________________
>     dev mailing list
>     dev at openvswitch.org
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=nAHjQgAdipmU4BHowXyPhoSGZvhBYs0Gprz21Toem1c&s=wrTPUsNoG_MEIRSjPdI7ExOiyaQh-f8tqCJLyyWrlo8&e= 
>     
> 
> 
> 
> 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev





More information about the dev mailing list