On Mon, Feb 29, 2016 at 11:25:43PM -0800, Justin Pettit wrote:
>
> > On Feb 19, 2016, at 4:40 PM, Ben Pfaff <[email protected]> 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
