On Mon, Feb 29, 2016 at 11:25:43PM -0800, Justin Pettit wrote: > > > On Feb 19, 2016, at 4:40 PM, Ben Pfaff <b...@ovn.org> wrote: > > > > +pinctrl_handle_arp(const struct flow *ip_flow, struct ofpbuf *userdata) > > { > > ... > > + if (ip_flow->vlan_tci) { > > + eth_push_vlan(&packet, htons(ETH_TYPE_VLAN_8021Q), > > ip_flow->vlan_tci); > > + } > > Not sure if it matters, since "ip_flow" should be pretty clean, but I > think we often check for vlans with "ip_flow->vlan_tci & > htons(VLAN_CFI)".
OK, I made the change. > > +static void > > +process_packet_in(const struct ofp_header *msg) > > ... > > + struct dp_packet packet; > > + dp_packet_use_const(&packet, pin.packet, pin.packet_len); > > + struct flow headers; > > + flow_extract(&packet, &headers); > > + > > + const struct flow *md = &pin.flow_metadata.flow; > > + switch (ntohl(ah->opcode)) { > > + case ACTION_OPCODE_ARP: > > + pinctrl_handle_arp(&headers, &userdata); > > There's an assumption in pinctrl_handle_arp() that "headers" is an IP > packet, but I don't see any verification of that here or in that > function. True. I added a check: /* This action only works for IP packets, and the switch should only send * us IP packets this way, but check here just to be sure. */ if (ip_flow->dl_type != htons(ETH_TYPE_IP)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); VLOG_WARN_RL(&rl, "ARP action on non-IP packet (Ethertype %"PRIx16")", ntohs(ip_flow->dl_type)); return; } > > ) > > +pinctrl_recv(const struct ofp_header *oh, enum ofptype type) > > { > > if (type == OFPTYPE_ECHO_REQUEST) { > > queue_msg(make_echo_reply(oh)); > > } else if (type == OFPTYPE_GET_CONFIG_REPLY) { > > + /* Enable asynchronous messages. */ > > struct ofputil_switch_config config; > > > > ofputil_decode_get_config_reply(oh, &config); > > config.miss_send_len = UINT16_MAX; > > set_switch_config(swconn, &config); > > Since it's not obvious that turning on a non-zero miss_send_len > enables async messages, it might be worth referring people to > DESIGN.md. OK, I improved the comment: /* Enable asynchronous messages (see "Asynchronous Messages" in * DESIGN.md for more information). */ > > static void > > +parse_arp_action(struct action_context *ctx) > > +{ > > ... > > + /* controller. */ > > It might be nice to say a little more about sending this to the > controller, since it doesn't fit the other comment styles in this > section. I think I forgot to finish the comment. I replaced it with: /* Add a "controller" action with the actions nested inside "arp {...}", * converted to OpenFlow, as its userdata. ovn-controller will convert the * packet to an ARP and then send the packet and actions back to the switch * inside an OFPT_PACKET_OUT message. */ _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev