[ovs-dev] [PATCH v2 0/4] OVN DHCP support proposal

[Ryan Moats]

Numan Siddique <nusiddiq at redhat.com> wrote on 06/15/2016 11:43:42 AM:

> From: Numan Siddique <nusiddiq at redhat.com>
> To: Ryan Moats/Omaha/IBM at IBMUS
> Cc: ovs dev <dev at openvswitch.org>
> Date: 06/15/2016 11:43 AM
> Subject: Re: [ovs-dev] [PATCH v2 0/4] OVN DHCP support proposal
>
> On Wed, Jun 15, 2016 at 8:44 PM, Ryan Moats <rmoats at us.ibm.com> wrote:
> "dev" <dev-bounces at openvswitch.org> wrote on 06/15/2016 04:16:44 AM:
>
> > From: Numan Siddique <nusiddiq at redhat.com>
> > To: ovs dev <dev at openvswitch.org>
> > Date: 06/15/2016 04:17 AM
> > Subject: [ovs-dev] [PATCH v2 0/4] OVN DHCP support proposal
> > Sent by: "dev" <dev-bounces at openvswitch.org>
>
> >
> > v1 -> v2 changes
> > ----------------
> >
> >  * Rebased the patches
> >
> >  * Patches 1 and 2 are from Ben and no changes. So not posting again.
> >     Please see
> >        * https://patchwork.ozlabs.org/patch/632633/
> >        * https://patchwork.ozlabs.org/patch/632634/
> >
> >  * Patch 3 changes
> >        * Deleted the extra test case "put_dhcp_opts" in ovn.at as it
> > is not required now
> >        * Deleted the tests/test-ovn-dhcp.c
> >
> >   * Patch 4 changes
> >        * Resolved the merge conflicts.
> >        * In the function "build_dhcp_aciton" changed
> >     -    ds_put_format(pause_action, "put_dhcp_opts(reg0, offerip =
> > "IP_FMT", ",
> >     +    ds_put_format(pause_action, "reg0[0] =
> > put_dhcp_opts(offerip = "IP_FMT", ",
> >                    IP_ARGS(offer_ip));
> >
> >        * Fixed the dhcp test case failure
> >
> >
> > v1
> > ---
> > Patches 1 and 2 are new.
> >
> > Patch 3 is based on https://patchwork.ozlabs.org/patch/631320/, with:
> >   - Minor style fixes.
> >   - Change syntax of put_dhcp_opts(), from:
> >       put_dhcp_opts(reg0, offerip=1.2.3.4, ...)
> >     to:
> >       reg0[0] = put_dhcp_opts(offerip=1.2.3.4, ...)
> >     That is, the result is now expressed as a return value, which is
> >     more natural for people coming from C, and the result is now a
> >     single bit instead of an entire register, which makes more sense
> >     for a Boolean anyway and doesn't use up a whole register.
> >   - Arguments to put_dhcp_opts are now architecture independent instead
> >     of host-endian, which makes it possible to test them using the
> >     test-ovn "parse-actions" instead of a separate program.
> >   - Added negative tests for put_dhcp_opts parsing.
> >   - Revised documentation to avoid talking about "pausing" and
"resuming"
> >     the pipeline.  The trip to ovn-controller should be transparent to
> >     the writer of the OVN logical flows.
> >
> > Patch 4 is based on https://patchwork.ozlabs.org/patch/631321/, with:
> >   - Minor style fixes.
> >   - Adapt actions to changed put_dhcp_opts() syntax.
> >   - Revised ovn-northd and documentation to avoid talking about
> >     "pausing" and "resuming" the pipeline.  The trip to ovn-controller
> >     should be transparent to the writer of the OVN logical flows.
> >
> > ---------------------
> > Ben Pfaff (2):
> >   expr: Shorten declarations of expr_context.
> >   expr: Refactor parsing of assignments and exchanges.
> >
> > Numan Siddique (2):
> >   ovn-controller: Add 'put_dhcp_opts' action in ovn-controller
> >   ovn-northd: Add logical flows to support native DHCP

> While I'm not entirely comfortable with mixing v1 and v2 patches,
>
> In the next series (in case if it is required), will post all the
> patches as part of the series :)
> ​
>
> I've run through and made sure this all applies and passes checks.
> I know I've complained about the goto in part 2 of v1 patch series
> elsewhere, so this can be considered a bulk ack for parts 3 and 4 of
> the revised series:

>
> ​Thanks. I used "goto" since it is used in many places. What other
> alternative you normally would suggest ? do, while (0) ? or something
else.

I was actually complaining about the goto in Ben's part 2 as I didn't
see that it bought me a lot in terms of cyclomatic complexity.
I didn't complain about yours because to remove them would end up adding
much cyclomatic complexity...

> Acked-by: Ryan Moats <rmoats at us.ibm.com>
> ​Thanks for the Acks​

yw

Ryan