Looks like a win, I've folded it in. Thanks.
Ethan
On Wed, Jun 26, 2013 at 2:45 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Wed, Jun 26, 2013 at 02:13:03PM -0700, Ben Pfaff wrote:
>> On Mon, Jun 24, 2013 at 06:59:26PM -0700, Ethan Jackson wrote:
>> > connmgr_must_output_local() requires a 'struct connmgr' handle,
>> > when in principle, it should simply be enough to know whether or
>> > not in_band is enabled. Breaking this up will allow
>> > ofproto-dpif-xlate to disentangle itself from ofproto-dpif in future
>> > patches.
>> >
>> > Signed-off-by: Ethan Jackson <et...@nicira.com>
>>
>> This is an improvement, thanks.
>>
>> Acked-by: Ben Pfaff <b...@nicira.com>
>
> Here's a patch you might consider insert in the series after that one.
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Ben Pfaff <b...@nicira.com>
> Date: Wed, 26 Jun 2013 14:44:39 -0700
> Subject: [PATCH] ofproto-dpif: Refactor checking for in-band special case.
>
> The comments on in_band_rule_check() were more or less wrong (the return
> value was no longer used to determine whether a flow could be set up).
> This commit fixes the comments and refactors the interface to make better
> sense in the current context.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/in-band.c | 29 +++++++----------------------
> ofproto/in-band.h | 5 ++---
> ofproto/ofproto-dpif-xlate.c | 23 ++++++++++++++++++-----
> 3 files changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/ofproto/in-band.c b/ofproto/in-band.c
> index 0746bab..03d4661 100644
> --- a/ofproto/in-band.c
> +++ b/ofproto/in-band.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -222,31 +222,16 @@ refresh_local(struct in_band *ib)
> return true;
> }
>
> -/* Returns true if the rule that would match 'flow' with 'actions' is
> - * allowed to be set up in the datapath. */
> +/* Returns true if packets in 'flow' should be directed to the local port.
> + * (This keeps the flow table from preventing DHCP replies from being seen by
> + * the local port.) */
> bool
> -in_band_rule_check(const struct flow *flow, odp_port_t local_odp_port,
> - const struct nlattr *actions, size_t actions_len)
> +in_band_must_output_to_local_port(const struct flow *flow)
> {
> - /* Don't allow flows that would prevent DHCP replies from being seen
> - * by the local port. */
> - if (flow->dl_type == htons(ETH_TYPE_IP)
> + return (flow->dl_type == htons(ETH_TYPE_IP)
> && flow->nw_proto == IPPROTO_UDP
> && flow->tp_src == htons(DHCP_SERVER_PORT)
> - && flow->tp_dst == htons(DHCP_CLIENT_PORT)) {
> - const struct nlattr *a;
> - unsigned int left;
> -
> - NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
> - if (nl_attr_type(a) == OVS_ACTION_ATTR_OUTPUT
> - && nl_attr_get_odp_port(a) == local_odp_port) {
> - return true;
> - }
> - }
> - return false;
> - }
> -
> - return true;
> + && flow->tp_dst == htons(DHCP_CLIENT_PORT));
> }
>
> static void
> diff --git a/ofproto/in-band.h b/ofproto/in-band.h
> index 5449312..ad16dc2 100644
> --- a/ofproto/in-band.h
> +++ b/ofproto/in-band.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -40,7 +40,6 @@ void in_band_set_remotes(struct in_band *,
> bool in_band_run(struct in_band *);
> void in_band_wait(struct in_band *);
>
> -bool in_band_rule_check(const struct flow *, odp_port_t local_odp_port,
> - const struct nlattr *odp_actions, size_t
> actions_len);
> +bool in_band_must_output_to_local_port(const struct flow *);
>
> #endif /* in-band.h */
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index cb429ef..a542995 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1831,6 +1831,22 @@ xlate_out_copy(struct xlate_out *dst, const struct
> xlate_out *src)
> src->odp_actions.size);
> }
>
> +static bool
> +actions_output_to_local_port(const struct xlate_ctx *ctx)
> +{
> + odp_port_t local_odp_port = ofp_port_to_odp_port(ctx->ofproto,
> OFPP_LOCAL);
> + const struct nlattr *a;
> + unsigned int left;
> +
> + NL_ATTR_FOR_EACH_UNSAFE (a, left, ctx->xout->odp_actions.data,
> + ctx->xout->odp_actions.size) {
> + if (nl_attr_type(a) == OVS_ACTION_ATTR_OUTPUT
> + && nl_attr_get_odp_port(a) == local_odp_port) {
> + return true;
> + }
> + }
> + return false;
> +}
>
> /* Translates the 'ofpacts_len' bytes of "struct ofpacts" starting at
> 'ofpacts'
> * into datapath actions in 'odp_actions', using 'ctx'. */
> @@ -1963,7 +1979,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> } else {
> static struct vlog_rate_limit trace_rl = VLOG_RATE_LIMIT_INIT(1, 1);
> size_t sample_actions_len;
> - odp_port_t local_odp_port;
>
> if (flow->in_port.ofp_port
> != vsp_realdev_to_vlandev(ctx.ofproto, flow->in_port.ofp_port,
> @@ -2001,11 +2016,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> }
> }
>
> - local_odp_port = ofp_port_to_odp_port(ctx.ofproto, OFPP_LOCAL);
> if (connmgr_has_in_band(ctx.ofproto->up.connmgr)
> - && !in_band_rule_check(flow, local_odp_port,
> - ctx.xout->odp_actions.data,
> - ctx.xout->odp_actions.size)) {
> + && in_band_must_output_to_local_port(flow)
> + && !actions_output_to_local_port(&ctx)) {
> compose_output_action(&ctx, OFPP_LOCAL);
> }
> if (mirror_ofproto_has_mirrors(ctx.ofproto)) {
> --
> 1.7.2.5
>
X-CudaMail-Whitelist-To: dev@openvswitch.org
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
