[ovs-dev] [PATCH 1/4] dpif-xlate : Refactoring translation of patch port output actions.

[Chandran, Sugesh]

Hi Joe,
Thank you for looking into the patch series.
 Please find my answers inline below.



Regards
_Sugesh


> -----Original Message-----
> From: Joe Stringer [mailto:joe at ovn.org]
> Sent: Monday, June 26, 2017 6:25 PM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>
> Cc: ovs dev <dev at openvswitch.org>; Zoltán Balogh
> <zoltan.balogh at ericsson.com>
> Subject: Re: [PATCH 1/4] dpif-xlate : Refactoring translation of patch port
> output actions.
> 
> On 16 June 2017 at 09:45, Sugesh Chandran <sugesh.chandran at intel.com>
> wrote:
> > Outputting to a patch port is translated by its peer patch port actions.
> > Refactoring the translation part to use later in the patch series for
> > the tunnel push.
> >
> > Signed-off-by: Sugesh Chandran <sugesh.chandran at intel.com>
> > Signed-off-by: Zoltán Balogh <zoltan.balogh at ericsson.com>
> > Co-authored-by: Zoltán Balogh <zoltan.balogh at ericsson.com>
> 
> Hi Sugesh,
> 
> Thanks for splitting this out into a separate patch. However, I notice that this
> patch makes some subtle changes to the logic between the set of code being
> removed and the set of code being added. Please don't do this; refactoring
> patches should be purely cosmetic, moving the exact code from one place to
> another. Then, to make subsequent changes to the logic, those should be in
> a separate patch with clear reasoning.
[Sugesh] We haven’t changed any logic between these two block. Only change that we
made is, instead of using the peer, we use the out-port param.
This allows us to use the same logic for the tunneling. 
Can you point out what logic change you are specifically referring to.
> This helps reviewers because we don't have to try to reason about subtle
> changes to behaviour in the middle of large code block moves, and it helps
> developers later on if there needs to be bisection, because the patches
> which potentially make behaviour changes are separated from cosmetic
> changes. Narrowing down which patch introduced a behaviour change is
> easier when it's not hidden in a refactor.
> 
> A trivial comment on patch titles: usually we use the imperative form, as
> described here:
> http://docs.openvswitch.org/en/latest/internals/contributing/submitting-
> patches/#email-subject
[Sugesh] Ok, Will change in the next series of patch.
> 
> For the module, "ofproto-dpif-xlate" can be pretty long, I see that in the past
> people have shortened to "xlate" to save characters.
> 
> So, the title could be more tersely, "xlate: Refactor translation of patch port
> output actions."
[Sugesh] Sure, will change it in v2
> 
> I have one more minor comment below.
> 
> > ---
> >  ofproto/ofproto-dpif-xlate.c | 270
> > ++++++++++++++++++++++---------------------
> >  1 file changed, 136 insertions(+), 134 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c
> > b/ofproto/ofproto-dpif-xlate.c index e15e3de..3a2b1d3 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -426,6 +426,10 @@ static void xlate_action_set(struct xlate_ctx
> > *ctx);  static void xlate_commit_actions(struct xlate_ctx *ctx);
> >
> >  static void
> > +apply_nested_clone_actions(struct xlate_ctx *ctx, const struct xport
> *in_dev,
> > +              struct xport *out_dev);
> > +
> > +static void
> >  ctx_trigger_freeze(struct xlate_ctx *ctx)  {
> >      ctx->exit = true;
> > @@ -3255,6 +3259,136 @@ xlate_flow_is_protected(const struct xlate_ctx
> *ctx, const struct flow *flow, co
> >              xport_in->xbundle->protected &&
> > xport_out->xbundle->protected);  }
> >
> > +/* Populate and apply nested actions on 'out_dev'.
> > + * The nested actions are applied on cloned packets in dp while
> > +outputting to
> > + * either patch or tunnel ports.
> 
> Where does the packet clone occur? I think I saw some discussion with Andy
> on this previously, but I'm not seeing it right now. If the packets are not
> actually being cloned, then at the least I think that this description should
> more clearly describe what is happening. I see other parts of the code
> describing something like 'this is equivalent to cloning the packet for the
> duration of execution'. Alternatively you could describe this function as
> pushing the OpenFlow state onto the stack, clearing it out,  executing the
> packet through the device, then finally popping the OpenFlow state back off
> the stack.
[Sugesh] The name clone comes from the past combine patch series. 
It basically combine the actions from a different device into current one.
We could add description saying its equivalent to cloning packets at the time of translation.
Will add it in the next series of patch.