[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