On Thu, Oct 13, 2011 at 3:23 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
> Almost all current actions can be expressed in the form of
> push/pop/set <field>, where field is one of the match fields. We can
> create three base actions and take a field. This has both a nice
> symmetry and avoids inconsistencies where we can match on the vlan
> TPID but not set it.
> Following patch converts all actions to this new format.
>
> Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
> Bug #7115
Overall, this looks pretty good to me. I have a few miscellaneous
comments but in general I like the structure of it. Ben, can you take
a look at it as well (although you might as well wait until Pravin has
had a chance to address my comments here)?
> diff --git a/datapath/actions.c b/datapath/actions.c
> index a28e986..c348daa 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -99,6 +99,8 @@ static int pop_vlan(struct sk_buff *skb)
>
> static int push_vlan(struct sk_buff *skb, __be16 new_tci)
> {
> + new_tci &= ~htons(VLAN_CFI_MASK);
It's better to use VLAN_TAG_PRESENT instead of VLAN_CFI_MASK. They
have the same value but the former better indicates why you're doing
it.
However, looking at the flow setup code more closely, I now see that I
misled you here - we actually don't set VLAN_TAG_PRESENT in the
communication between userspace and kernel - it's instead implied by
the presence of the Netlink attribute. Both sides use this convention
internally but it is masked out when serializing the data.
Given that, I would mask it out in userspace and then add a check to
the validation code for it.
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index b3e2442..f520502 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> +static int validate_set_actions(const struct nlattr *ovs_key,
> + const struct sw_flow_key *flow_key)
> +{
[...]
> + default:
> + return -EOPNOTSUPP;
I think it's better to just return -EINVAL here (and in the main
validation_actions() function as well). Otherwise you end up with
different error codes when trying to set an IPv4 protocol type vs. an
IPv6 type.
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index c077f62..2bc7a00 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> +/**
> + * OVS datapath actions are expressed using enum ovs_action_type and enum
> + * ovs_key_type. enum ovs_action_type is like base action and ovs_key_type
> + * specifies which field to act on. i.e. push/pop/set <field>.
> + * E.g. @OVS_ACTION_ATTR_SET could have nested netlink attribute of type
> + * %OVS_KEY_ATTR_TUN_ID, %OVS_KEY_ATTR_ETHERNET, %OVS_KEY_ATTR_IPV4,
> + * %OVS_KEY_ATTR_TCP or %OVS_KEY_ATTR_UDP.
> + */
I found this comment somewhat confusing to read. Maybe you could just
give a single example of what an action would look like instead of
talking about the two pieces separately?
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 9967322..05f72e3 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> +dp_netdev_set_udp_port(struct ofpbuf *packet, const struct flow *flow,
> + const struct ovs_key_udp *udp_key)
> +{
> + /* Check for trasport header. */
> + if (packet->l7 && flow->nw_proto == IPPROTO_UDP) {
I don't think any of the tests of this form (with the exception of the
pseudoheader changing when the IP address changes) are necessary
because the main userspace should be able to tell whether the packet
headers are valid or not. If it doesn't then when used with the
kernel module the flows will be rejected.
> +static void
> dp_netdev_execute_actions(struct dp_netdev *dp,
> struct ofpbuf *packet, struct flow *key,
> const struct nlattr *actions,
[...]
> - case OVS_ACTION_ATTR_SET_TUNNEL:
You need to still include this in execute_set_action() as a no-op
because it's still possible for the set_tunnel OpenFlow command to be
used in this case. When used without a tunnel (as is always the case
here), there's no effect but after this patch it will cause an
assertion failure.
> diff --git a/lib/netlink.c b/lib/netlink.c
> index b4de3ed..f90a0de 100644
> --- a/lib/netlink.c
> +++ b/lib/netlink.c
> @@ -333,6 +333,18 @@ nl_msg_put_nested(struct ofpbuf *msg,
> nl_msg_end_nested(msg, offset);
> }
>
> +void
> +nl_msg_put_nested_attr(struct ofpbuf *odp_actions, uint16_t type,
> + uint16_t n_type, const void *n_data, size_t n_size)
Where does the n_ prefix come from? It's somewhat confusing because
it's usually used for number of X. I think this function doesn't
really belong in netlink.c since it's not generic code but is actually
specific to inserting actions.
> + void *attr;
> +
> + attr = nl_msg_put_unspec_uninit(odp_actions, n_type, n_size);
> + memcpy(attr, n_data, n_size);
I think you can use nl_msg_put_unspec() in place of these three lines.
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index a471099..042a1d5 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> +static void
> format_odp_action(struct ds *ds, const struct nlattr *a)
> {
[...]
> + case OVS_ACTION_ATTR_POP:
> + if (nl_attr_get_u16(a) == OVS_KEY_ATTR_8021Q)
> + ds_put_cstr(ds, "pop_vlan");
For consistency can we use "pop(vlan)"?
> + else
> + ds_put_format(ds, "pop(%"PRIu16")", nl_attr_get_u16(a));
When we format unknown actions in other places we use the prefix "key"
as in "pop(key%"PRIu16")".
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 8e5a863..0ada375 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> +static void
> +commit_set_nw_action(const struct flow *flow, struct flow *base,
> + struct ofpbuf *odp_actions)
> +{
> + struct ovs_key_ipv4 ipv4_key;
>
> - if (base->nw_dst != flow->nw_dst) {
> - nl_msg_put_be32(odp_actions, OVS_ACTION_ATTR_SET_NW_DST,
> flow->nw_dst);
> - base->nw_dst = flow->nw_dst;
> + if (flow->dl_type != htons(ETH_TYPE_IP) ||
> + !flow->nw_src || !flow->nw_src ) {
I think that second comparison should be nw_dst. There's also an
extra space before the last parenthesis.
> +static void
> +commit_set_port_action(const struct flow *flow, struct flow *base,
> + struct ofpbuf *odp_actions)
> +{
> + if (!flow->tp_src || !flow->tp_src) {
Another src || src.
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index b5ca08c..13c1435 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -75,7 +75,7 @@ in_port=5 actions=set_tunnel:5
> AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> AT_CHECK([ovs-appctl -t test-openflowd ofproto/trace br0
> 'tun_id(0x1),in_port(90),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0),icmp(type=8,code=0)'],
> [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
> - [Datapath actions: set_tunnel(0x1),1,2,set_tunnel(0x3),3,4
> + [Datapath actions: set(tun_id(0x1)),1,2,set(tun_id(0x3)),3,4
You also need to update test 387: ofproto-dpif.at:83 ofproto-dpif -
VLAN handling to account for the changed output.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
