[ovs-dev] [PATCH v5 1/5] actions: Implement OVN "arp" action.

Justin Pettit jpettit at ovn.org
Tue Mar 1 07:25:43 UTC 2016


> On Feb 19, 2016, at 4:40 PM, Ben Pfaff <blp at 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)".

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

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

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

--Justin





More information about the dev mailing list