[ovs-dev] [PATCH] ofproto-dpif: Fix up user specifying wrong bridge on "ofproto/trace".

Ben Pfaff blp at nicira.com
Fri Mar 8 18:39:18 UTC 2013


It doesn't seem like a big deal to me one way or another.  Anyway, if
it's a problem I'm sure we'll get a report.

I applied this to master and branch-1.10.  Thanks for the review.

On Fri, Mar 08, 2013 at 10:20:24AM -0800, Justin Pettit wrote:
> I wonder if it would be better to only print the bridge name when
> there was a mismatch along with a warning.  I'm not a heavy user of
> "ofproto/trace", so I don't have a good sense for which way would be
> better.
> 
> Looks good, though.
> 
> --Justin
> 
> 
> On Mar 5, 2013, at 4:49 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > If there is more than one bridge, then it's easy to specify the wrong one
> > on an ofproto/trace command.  Previously, this would produce surprising
> > results.  With this commit, "ofproto/trace" should silently fix up the
> > problem.
> > 
> > It would be nice to not require the user to specify a bridge at all, but
> > it's theoretically possible to have more than one backer, in which case we
> > need some way to distinguish, and a bridge name is as good an identifier
> > as we have.  We could ask the user to specify the datapath_type, I guess,
> > but that's a less familiar name to most users and it would be a somewhat
> > gratuitous change in synatx for ofproto/trace.
> > 
> > Bug #15419.
> > Reported-by: Paul Ingram <paul at nicira.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > ofproto/ofproto-dpif.c |   10 +++++-----
> > 1 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 7035530..1087d72 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -7626,16 +7626,16 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
> >                 goto exit;
> >             }
> > 
> > -            /* XXX: Since we allow the user to specify an ofproto, it's
> > -             * possible they will specify a different ofproto than the one the
> > -             * port actually belongs too.  Ideally we should simply remove the
> > -             * ability to specify the ofproto. */
> > +            /* The user might have specified the wrong ofproto but within the
> > +             * same backer.  That's OK, ofproto_receive() can find the right
> > +             * one for us. */
> >             if (ofproto_receive(ofproto->backer, NULL, odp_key.data,
> > -                                odp_key.size, &flow, NULL, NULL, NULL,
> > +                                odp_key.size, &flow, NULL, &ofproto, NULL,
> >                                 &initial_tci)) {
> >                 unixctl_command_reply_error(conn, "Invalid flow");
> >                 goto exit;
> >             }
> > +            ds_put_format(&result, "Bridge: %s\n", ofproto->up.name);
> >         } else {
> >             char *error_s;
> > 
> > -- 
> > 1.7.2.5
> > 
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> 



More information about the dev mailing list