[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