[ovs-dev] [InBandOVSDB 1/4] in-band: Generalize the in-band code to arbitrary (IP, port) pairs.

Ben Pfaff blp at nicira.com
Mon Apr 26 17:14:44 UTC 2010


On Sun, Apr 25, 2010 at 11:40:12PM -0700, Justin Pettit wrote:
> On Apr 20, 2010, at 4:37 PM, Ben Pfaff wrote:
> 
> >  *    d. ARP replies to the remote side's MAC address.  Note that the 
> > - *       remote side is either the controller or the gateway to reach 
> > - *       the controller.
> > + *       remote side is either the  or the gateway to reach 
> > + *       the remote.
> 
> I think "remote" is missing between "the" and "or".  This overloading
> of "remote" gets a bit confusing.  What do you think of changing
> references to the original "remote" (which meant the gateway or remote
> side) to "next hop".

You're right, I didn't proofread this description very well.  I reworded
this section this way:

 * In-band also sets up the following rules for each unique next-hop MAC
 * address for the remotes' IPs (the "next hop" is either the remote
 * itself, if it is on a local subnet, or the gateway to reach the remote):
 * 
 *    d. ARP replies to the next hop's MAC address.
 *    e. ARP requests from the next hop's MAC address.

Is that better?

> > @@ -163,27 +175,27 @@
> >  *
> >  * The following are explicitly *not* supported by in-band control:
> >  *
> > - *    - Specify Controller by Name.  Currently, the controller must be 
> > + *    - Specify Remote by Name.  Currently, the remote must be 
> >  *      identified by IP address.  A naive approach would be to permit
> >  *      all DNS traffic.  Unfortunately, this would prevent the
> > - *      controller from defining any policy over DNS.  Since switches
> > - *      that are located behind us need to connect to the controller, 
> > + *      remote from defining any policy over DNS.  Since switches
> 
> I think this "remote" should be "controller".

Thanks, fixed.

> > @@ -199,10 +211,10 @@ enum {
> >     IBR_FROM_LOCAL_ARP,           /* (c) From local port, ARP. */
> >     IBR_TO_REMOTE_ARP,            /* (d) To remote MAC, ARP. */
> >     IBR_FROM_REMOTE_ARP,          /* (e) From remote MAC, ARP. */
> > -    IBR_TO_CTL_ARP,               /* (f) To controller IP, ARP. */
> > -    IBR_FROM_CTL_ARP,             /* (g) From controller IP, ARP. */
> > -    IBR_TO_CTL_OFP,               /* (h) To controller, OpenFlow port. */
> > -    IBR_FROM_CTL_OFP              /* (i) From controller, OpenFlow port. */
> > +    IBR_TO_CTL_ARP,               /* (f) To remote IP, ARP. */
> > +    IBR_FROM_CTL_ARP,             /* (g) From remote IP, ARP. */
> > +    IBR_TO_CTL_TCP,               /* (h) To remote IP, TCP port. */
> > +    IBR_FROM_CTL_TCP              /* (i) From remote IP, TCP port. */
> 
> We should probably rename those "CTL"s to be something else.  Once
> again, we should probably be a bit more careful of our overloading of
> "remote" to mean both controller/manager and gateway.  What about
> renaming the existing "REMOTE"s to "NEXT_HOP"?

You're right.  The names are no longer very good.

Here's what I ended up with:

enum {
    /* One set per bridge. */
    IBR_FROM_LOCAL_DHCP = 180000, /* (a) From local port, DHCP. */
    IBR_TO_LOCAL_ARP,             /* (b) To local port, ARP. */
    IBR_FROM_LOCAL_ARP,           /* (c) From local port, ARP. */

    /* One set per unique next-hop MAC. */
    IBR_TO_NEXT_HOP_ARP,          /* (d) To remote MAC, ARP. */
    IBR_FROM_NEXT_HOP_ARP,        /* (e) From remote MAC, ARP. */

    /* One set per unique remote IP address. */
    IBR_TO_REMOTE_ARP,            /* (f) To remote IP, ARP. */
    IBR_FROM_REMOTE_ARP,          /* (g) From remote IP, ARP. */

    /* One set per unique remote (IP,port) pair. */
    IBR_TO_REMOTE_TCP,            /* (h) To remote IP, TCP port. */
    IBR_FROM_REMOTE_TCP           /* (i) From remote IP, TCP port. */
};


> > @@ -495,14 +494,14 @@ static void
> > set_tp_src(struct in_band_rule *rule, uint16_t tp_src)
> > {
> >     rule->wildcards &= ~OFPFW_TP_SRC;
> > -    rule->flow.tp_src = htons(tp_src);
> > +    rule->flow.tp_src = tp_src;
> > }
> 
> It looks like the set_xxx() arguments are now expected to take their
> argument in network-byte order.  However, set_dl_type() still takes
> its argument in host-byte order.  For consistency, I think having the
> argument be in network-byte order would be cleaner.  But if you don't
> want to do that, then I think it should at least be documented.

OK, I changed them to all use network byte order.

> > +    for (i = 0; i < ib->n_remote_addrs; i++) {
> > +        const struct sockaddr_in *a = &ib->remote_addrs[i];
> > +
> > +        if (!i || a->sin_addr.s_addr != a[-1].sin_addr.s_addr) {
> > +            /* Allow ARP replies to the controller's IP. */
> 
> This should probably be "the remote's IP".

OK, I changed all of the comments on these flows to match the ones at
the top of the file.  Thanks.

> > +        if (!i ||
> > +            a->sin_addr.s_addr != a[-1].sin_addr.s_addr ||
> 
> I prefer having the binary operators on a new line, too.  :-)

OK, I'll stick by my guns then.  I guess this is just a personal
preference thing, no reason to militate it.




More information about the dev mailing list