On Tue, Oct 18, 2011 at 3:47 PM, Jesse Gross <je...@nicira.com> wrote:
> 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.
>
ok,
>> 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.
>
ok
>> 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?
>
ok
>> 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.
>
ok
>> +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.
right, i missed it.
>
>> 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.
>
ok, I will move it to ofproo-dpif.
>> + 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.
>
ok
>> 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)"?
>
ok
>> + 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.
>
right, vlan test was skipped in my make check.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
