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

Justin Pettit jpettit at nicira.com
Mon Apr 26 06:40:12 UTC 2010


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".

> @@ -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".

> @@ -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"?

> @@ -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.

> +    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".

> +            init_rule(&rule, IBR_TO_CTL_ARP);
> +            set_dl_type(&rule, ETH_TYPE_ARP);
> +            set_nw_proto(&rule, ARP_OP_REPLY);
> +            set_nw_dst(&rule, a->sin_addr);
> +            cb(ib, &rule);
> +
> +            /* Allow ARP requests from the controller's IP. */

Same here.

> +            init_rule(&rule, IBR_FROM_CTL_ARP);
> +            set_dl_type(&rule, ETH_TYPE_ARP);
> +            set_nw_proto(&rule, ARP_OP_REQUEST);
> +            set_nw_src(&rule, a->sin_addr);
> +            cb(ib, &rule);
> ...
> +        if (!i ||
> +            a->sin_addr.s_addr != a[-1].sin_addr.s_addr ||

I prefer having the binary operators on a new line, too.  :-)

--Justin






More information about the dev mailing list