[ovs-dev] [L3 In-Band 9/9] in-band: Implement L3-based in-band control

Ben Pfaff blp at nicira.com
Tue Sep 1 22:57:27 UTC 2009


Justin Pettit <jpettit at nicira.com> writes:

> What do you think of adding Lolcat variable name requirement in the
> Coding Style document?  For example, "have_mac" would be changed to
> "i_has_mac".  Well, think about it.  Regardless, I made the requested
> change.

lolcat is an Internetism that I was previously unaware of.  I
don't have time to look at all 532 pages, can you recommend just
a few?

>>> +    /* Regardless of how the flow table is configured, we want to be
>>> +     * able to see replies to our DHCP requests. */
>>> +    if ((flow->nw_proto == IP_TYPE_UDP)
>>> +            && (flow->tp_src == htons(DHCP_SERVER_PORT))
>>> +            && (flow->tp_dst == htons(DHCP_CLIENT_PORT))
>>> +            && (packet->size >= DHCP_HEADER_LEN)) {
>>> +        struct dhcp_header *dhcp = packet->l7;
>>
>> Shouldn't this check for dl_type == htons(ETH_TYPE_IP)?
>
> I don't think it's strictly required, because flow_extract() (which
> ran before this), zeros out the flow structure.  However, I added it
> for clarity.  I also removed the extra parens, so if any kernel mavens
> peruse the code that they don't vomit.

Thank you.

I agree that it isn't necessary as flow_extract() is currently
implemented.  The reason to do it is for future-proofing, in case
some protocol other than IPv4 sticks a value that is not
IP-compatible in the nw_proto member.

>>> -    /* Switch traffic sent by the local port. */
>>> +    /* Allow DHCP requests to be sent from the local port. */
>>>     memset(&flow, 0, sizeof flow);
>>>     flow.in_port = ODPP_LOCAL;
>>> -    setup_flow(in_band, IBR_FROM_LOCAL_PORT, &flow, OFPFW_IN_PORT,
>>> -               OFPP_NORMAL);
>>> +    flow.dl_type = htons(ETH_TYPE_IP);
>>> +    flow.nw_proto = IP_TYPE_UDP;
>>> +    flow.tp_src = htons(DHCP_CLIENT_PORT);
>>> +    flow.tp_dst = htons(DHCP_SERVER_PORT);
>>> +    setup_flow(in_band, IBR_FROM_LOCAL_DHCP, &flow,
>>> +               (OFPFW_IN_PORT | OFPFW_DL_TYPE | OFPFW_NW_PROTO
>>> +                | OFPFW_TP_SRC | OFPFW_TP_DST), OFPP_NORMAL);
>>
>> Can we fix dl_src as local_mac here?
>
> Probably.  Since we've bound it to the local port, I didn't know if
> was strictly necessary.  I've added it, though.

It isn't strictly necessary.  It might make life easier for the
classifier though.  That's the only reason to include it.

>>
>> ...to "set up" this flow.
>
> Shouldn't the function you wrote "setup_flow" be "set_up_flow"?  ;-)

Yes.  Oops.




More information about the dev mailing list