Thanks for the checking.
I will correct them. Do you know any similar script for user space codes?
Thanks a lot.
Wenyu
在 2014-7-23，5:29，"Pravin Shelar" <pshe...@nicira.com> 写道：
> I have seen multiple coding style errors and warning from kernel
> checkpatch.pl on kernel part of the patch:-
>
> WARNING: Missing a blank line after declarations
> #88: FILE: datapath/actions.c:530:
> + struct vport *vport;
> + vport = ovs_vport_ovsl_rcu(dp, nla_get_u32(a));
>
> ERROR: space required before the open brace '{'
> #89: FILE: datapath/actions.c:531:
> + if (vport->ops->get_out_tun_info){
>
> WARNING: braces {} are not necessary for single statement blocks
> #92: FILE: datapath/actions.c:534:
> + if (err == 0) {
> + upcall.out_tun_info = &output_tunnel_info;
> + }
>
> WARNING: line over 80 characters
> #93: FILE: datapath/actions.c:535:
> + upcall.out_tun_info = &output_tunnel_info;
>
> ERROR: space required after that close brace '}'
> #99: FILE: datapath/actions.c:541:
> + }/* End of switch. */
>
> ERROR: code indent should use tabs where possible
> #175: FILE: datapath/datapath.c:432:
> + if (upcall_info->out_tun_info)$
>
> WARNING: please, no spaces at the start of a line
> #175: FILE: datapath/datapath.c:432:
> + if (upcall_info->out_tun_info)$
>
> ERROR: code indent should use tabs where possible
> #197: FILE: datapath/datapath.c:518:
> + }$
>
> WARNING: please, no spaces at the start of a line
> #197: FILE: datapath/datapath.c:518:
> + }$
>
> ERROR: code indent should use tabs where possible
> #231: FILE: datapath/flow.h:42:
> + (offsetof(struct ovs_key_ipv4_tunnel, tp_dst) + ^I\$
>
> WARNING: please, no space before tabs
> #231: FILE: datapath/flow.h:42:
> + (offsetof(struct ovs_key_ipv4_tunnel, tp_dst) + ^I\$
>
> WARNING: please, no spaces at the start of a line
> #231: FILE: datapath/flow.h:42:
> + (offsetof(struct ovs_key_ipv4_tunnel, tp_dst) + ^I\$
>
> ERROR: code indent should use tabs where possible
> #232: FILE: datapath/flow.h:43:
> + FIELD_SIZEOF(struct ovs_key_ipv4_tunnel, tp_dst))$
>
> WARNING: please, no spaces at the start of a line
> #232: FILE: datapath/flow.h:43:
> + FIELD_SIZEOF(struct ovs_key_ipv4_tunnel, tp_dst))$
>
> WARNING: line over 80 characters
> #255: FILE: datapath/flow.h:72:
> + __be32 saddr, __be32 daddr, u8 tos, u8 ttl,
>
> ERROR: code indent should use tabs where possible
> #345: FILE: datapath/flow_netlink.c:497:
> +^I^I^I const struct ovs_key_ipv4_tunnel *output,$
>
> ERROR: code indent should use tabs where possible
> #346: FILE: datapath/flow_netlink.c:498:
> +^I^I^I const struct geneve_opt *tun_opts,$
>
> ERROR: code indent should use tabs where possible
> #347: FILE: datapath/flow_netlink.c:499:
> +^I^I^I int swkey_tun_opts_len)$
>
> WARNING: braces {} are not necessary for single statement blocks
> #392: FILE: datapath/flow_netlink.c:552:
> + if (err) {
> + return err;
> + }
>
> ERROR: code indent should use tabs where possible
> #404: FILE: datapath/flow_netlink.c:564:
> +^I out_tun_info->options,$
>
> ERROR: code indent should use tabs where possible
> #405: FILE: datapath/flow_netlink.c:565:
> +^I out_tun_info->options_len);$
>
> ERROR: code indent should use tabs where possible
> #502: FILE: datapath/vport-gre.c:296:
> + struct ovs_tunnel_info * out_tun_info)$
>
> WARNING: please, no spaces at the start of a line
> #502: FILE: datapath/vport-gre.c:296:
> + struct ovs_tunnel_info * out_tun_info)$
>
> ERROR: "foo * bar" should be "foo *bar"
> #502: FILE: datapath/vport-gre.c:296:
> + struct ovs_tunnel_info * out_tun_info)
>
> WARNING: please, no space before tabs
> #522: FILE: datapath/vport-gre.c:311:
> +^I.send^I ^I^I= gre_send,$
>
> ERROR: code indent should use tabs where possible
> #555: FILE: datapath/vport-lisp.c:250:
> +^I key, TUNNEL_KEY, NULL, 0);$
>
> ERROR: code indent should use tabs where possible
> #564: FILE: datapath/vport-lisp.c:520:
> + struct ovs_tunnel_info * out_tun_info)$
>
> WARNING: please, no spaces at the start of a line
> #564: FILE: datapath/vport-lisp.c:520:
> + struct ovs_tunnel_info * out_tun_info)$
>
> ERROR: "foo * bar" should be "foo *bar"
> #564: FILE: datapath/vport-lisp.c:520:
> + struct ovs_tunnel_info * out_tun_info)
>
> ERROR: code indent should use tabs where possible
> #621: FILE: datapath/vport-vxlan.c:193:
> + struct ovs_tunnel_info * out_tun_info)$
>
> WARNING: please, no spaces at the start of a line
> #621: FILE: datapath/vport-vxlan.c:193:
> + struct ovs_tunnel_info * out_tun_info)$
>
> ERROR: "foo * bar" should be "foo *bar"
> #621: FILE: datapath/vport-vxlan.c:193:
> + struct ovs_tunnel_info * out_tun_info)
>
> WARNING: braces {} are not necessary for single statement blocks
> #630: FILE: datapath/vport-vxlan.c:202:
> + if (unlikely(!OVS_CB(skb)->tun_info)) {
> + return -EINVAL;
> + }
>
> WARNING: braces {} are not necessary for single statement blocks
> #693: FILE: datapath/vport.c:593:
> + if (IS_ERR(rt)) {
> + return PTR_ERR(rt);
> + }
>
> ERROR: code indent should use tabs where possible
> #743: FILE: datapath/vport.h:178:
> +^I^I^I struct ovs_tunnel_info *);$
>
> WARNING: line over 80 characters
> #772: FILE: include/linux/openvswitch.h:196:
> + OVS_PACKET_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
>
> WARNING: line over 80 characters
> #774: FILE: include/linux/openvswitch.h:198:
> + OVS_PACKET_ATTR_OUT_TUNNEL_KEY, /* Nested OVS_TUNNEL_KEY_ATTR_* attributes
> */
>
> WARNING: line over 80 characters
> #782: FILE: include/linux/openvswitch.h:349:
> + OVS_TUNNEL_KEY_ATTR_TP_SRC, /* u16 Tunnel Source Transport Port. */
>
> WARNING: line over 80 characters
> #783: FILE: include/linux/openvswitch.h:350:
> + OVS_TUNNEL_KEY_ATTR_TP_DST, /* u16 Tunnel Destination Transport Port. */
>
> ERROR: code indent should use tabs where possible
> #799: FILE: include/linux/openvswitch.h:525:
> + * to get tunnel info.$
>
> ERROR: code indent should use tabs where possible
> #800: FILE: include/linux/openvswitch.h:526:
> + */$
>
> total: 21 errors, 20 warnings, 655 lines checked
>
> -------------------
> Please fix all these issue in next revision.
>
> Thanks,
> Pravin.
>
> On Tue, Jul 22, 2014 at 1:02 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>> On Mon, Jul 21, 2014 at 8:35 PM, Wenyu Zhang <wen...@vmware.com> wrote:
>>> Thanks for the review, my comments inline.
>>>
>>> Bests,
>>> Wenyu
>>> -----Original Message-----
>>> From: Pravin Shelar [mailto:pshe...@nicira.com]
>>> Sent: Tuesday, July 22, 2014 6:56 AM
>>> To: Wenyu Zhang
>>> Cc: Romain Lenglet; Ben Pfaff; Jesse Gross; dev@openvswitch.org
>>> Subject: Re: [PATCH v5] Extend OVS IPFIX exporter to export tunnel headers
>>>
>>> On Mon, Jul 21, 2014 at 2:39 AM, Wenyu Zhang <wen...@vmware.com> wrote:
>>>> Extend IPFIX exporter to export tunnel headers when both input and output
>>>> of the port.
>>>> Add three other_config options in IPFIX table: enable-input-sampling,
>>>> enable-output-sampling and enable-tunnel-sampling, to control whether
>>>> sampling tunnel info, on which direction (input or output).
>>>> Insert sampling action before output action and the output tunnel port
>>>> is sent to datapath in the sampling action.
>>>> Make datapath collect output tunnel info and send it back to userpace
>>>> in upcall message with a new additional optional attribute.
>>>> Add a tunnel ports map to make the tunnel port lookup faster in sampling
>>>> upcalls in IPFIX exporter. Make the IPFIX exporter generate IPFIX template
>>>> sets with enterprise elements for the tunnel info, save the tunnel info
>>>> in IPFIX cache entries, and send IPFIX DATA with tunnel info.
>>>> Add flowDirection element in IPFIX templates.
>>>>
>>>> Signed-off-by: Wenyu Zhang <wen...@vmware.com>
>>>> Acked-by: Romain Lenglet <rleng...@vmware.com>
>>>
>>> This looking close, I have few comments below.
>>>
>>>> ---
>>>> v2: Address Romain's comments
>>>> v3: Address Pravin's comments, make datapath sent all tunnel info,
>>>> not only tunnel key to userspace.
>>>> v4: Address Pravin's comments, introduce a common function to get output
>>>> tunnel info for all vports, remove duplicated codes.
>>>> Rebase.
>>>> v5: Address Pravin's comments on v4, correct sparse errors, make a common
>>>> function to setup tunnel info data for both input and output case.
>>>> ---
>>>> datapath/actions.c | 18 ++
>>>> datapath/datapath.c | 46 +++-
>>>> datapath/datapath.h | 2 +
>>>> datapath/flow.h | 54 ++--
>>>> datapath/flow_netlink.c | 58 ++++-
>>>> datapath/flow_netlink.h | 2 +
>>>> datapath/vport-geneve.c | 36 ++-
>>>> datapath/vport-gre.c | 35 ++-
>>>> datapath/vport-lisp.c | 40 ++-
>>>> datapath/vport-vxlan.c | 39 ++-
>>>> datapath/vport.c | 40 +++
>>>> datapath/vport.h | 11 +
>>>> include/linux/openvswitch.h | 21 +-
>>>> lib/dpif-linux.c | 2 +
>>>> lib/dpif.h | 1 +
>>>> lib/odp-util.c | 183 +++++++++-----
>>>> lib/odp-util.h | 2 +
>>>> lib/packets.h | 9 +-
>>>> ofproto/automake.mk | 3 +
>>>> ofproto/ipfix-enterprise-entities.def | 19 ++
>>>> ofproto/ofproto-dpif-ipfix.c | 444
>>>> ++++++++++++++++++++++++++++++---
>>>> ofproto/ofproto-dpif-ipfix.h | 14 +-
>>>> ofproto/ofproto-dpif-upcall.c | 20 +-
>>>> ofproto/ofproto-dpif-xlate.c | 51 +++-
>>>> ofproto/ofproto-dpif.c | 20 ++
>>>> ofproto/ofproto.h | 3 +
>>>> ofproto/tunnel.c | 4 +
>>>> tests/odp.at | 9 +-
>>>> utilities/ovs-vsctl.8.in | 8 +-
>>>> vswitchd/bridge.c | 9 +
>>>> vswitchd/vswitch.ovsschema | 5 +-
>>>> vswitchd/vswitch.xml | 27 ++
>>>> 32 files changed, 1048 insertions(+), 187 deletions(-)
>>>> create mode 100644 ofproto/ipfix-enterprise-entities.def
>>>>
>>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>>> index 39a21f4..c1ad4dc 100644
>>>> --- a/datapath/actions.c
>>>> +++ b/datapath/actions.c
>>>> @@ -502,6 +502,7 @@ static int output_userspace(struct datapath *dp,
>>>> struct sk_buff *skb,
>>>> struct dp_upcall_info upcall;
>>>> const struct nlattr *a;
>>>> int rem;
>>>> + struct ovs_tunnel_info output_tunnel_info;
>>>>
>>>> BUG_ON(!OVS_CB(skb)->pkt_key);
>>>>
>>>> @@ -509,6 +510,7 @@ static int output_userspace(struct datapath *dp,
>>>> struct sk_buff *skb,
>>>> upcall.key = OVS_CB(skb)->pkt_key;
>>>> upcall.userdata = NULL;
>>>> upcall.portid = 0;
>>>> + upcall.out_tun_info = NULL;
>>>>
>>>> for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
>>>> a = nla_next(a, &rem)) {
>>>> @@ -520,7 +522,23 @@ static int output_userspace(struct datapath *dp,
>>>> struct sk_buff *skb,
>>>> case OVS_USERSPACE_ATTR_PID:
>>>> upcall.portid = nla_get_u32(a);
>>>> break;
>>>> +
>>>> + case OVS_USERSPACE_ATTR_TUNNEL_OUT_PORT: {
>>>> + /* Get out tunnel info. */
>>>> + int err;
>>>> + struct vport *vport;
>>>> + vport = ovs_vport_ovsl_rcu(dp, nla_get_u32(a));
>>> This function is never called under ovs_lock, so you can use rcu
>>> version of lookup.
>>> Wenyu: Sorry, I am confused. I have used ovs_vport_ovsl_rcu(), do you mean
>>> another rcu version of lookup?
>>
>> You can use ovs_vport_rcu().
>>
>>>> + if (vport->ops->get_out_tun_info){
>>>
>>> You also need to check for vport, if it is NULL.
>>> Wenyu: Sure, I will add check for vport and vport->ops.
>> ok.
>>
>>>> + err = vport->ops->get_out_tun_info(
>>>> + vport, skb, &output_tunnel_info);
>>>> + if (err == 0) {
>>>> + upcall.out_tun_info =
>>>> &output_tunnel_info;
>>>> + }
>>>> + }
>>>> + break;
>>>> }
>>>> +
>>>> + }/* End of switch. */
>>>> }
>>>
>>> .......
>>>> {
>>>> + BUILD_BUG_ON(sizeof(tun_info->tunnel) != OVS_TUNNEL_KEY_SIZE);
>>>> +
>>>> tun_info->tunnel.tun_id = tun_id;
>>>> - tun_info->tunnel.ipv4_src = iph->saddr;
>>>> - tun_info->tunnel.ipv4_dst = iph->daddr;
>>>> - tun_info->tunnel.ipv4_tos = iph->tos;
>>>> - tun_info->tunnel.ipv4_ttl = iph->ttl;
>>>> + tun_info->tunnel.ipv4_src = saddr;
>>>> + tun_info->tunnel.ipv4_dst = daddr;
>>>> + tun_info->tunnel.ipv4_tos = tos;
>>>> + tun_info->tunnel.ipv4_ttl = ttl;
>>>> tun_info->tunnel.tun_flags = tun_flags;
>>>>
>>>> - /* clear struct padding. */
>>>> - memset((unsigned char *) &tun_info->tunnel + OVS_TUNNEL_KEY_SIZE,
>>>> 0,
>>>> - sizeof(tun_info->tunnel) - OVS_TUNNEL_KEY_SIZE);
>>>
>>> We still need this memset for case where we add another member to this
>>> struct, you can check for size of padding to avoid sparse warning.
>>> Wenyu: The additional if condition may affect performance, we'd better to
>>> add some comments to mention this usage.
>>> I have added a compling check about the size:
>>> BUILD_BUG_ON(sizeof(tun_info->tunnel) != OVS_TUNNEL_KEY_SIZE);
>>> If we add another member to this structre in future, this
>>> check will failed when compling to mention us, and then we can add the
>>> memset only when the failure happens.
>>> And I can add some comments about the usage of memset here.
>>
>> Compiler can determine size is zero and it does not generate code if
>> it is not required. so there is no performance penalty.
>>
>>>> + /* For the tunnel types on the top of IPsec, the tp_src and tp_dst
>>>> of
>>>> + * the upper tunnel are used.
>>>> + * E.g: GRE over IPSEC, the tp_src and tp_port are zero.
>>>> + */
>>>> + tun_info->tunnel.tp_src = tp_src;
>>>> + tun_info->tunnel.tp_dst = tp_dst;
>>>>
>>>> tun_info->options = opts;
>>>> tun_info->options_len = opts_len;
>>>> }
>>> .....
>>>
>>>>
>>>> +static int geneve_get_out_tun_info(struct vport *vport, struct sk_buff
>>>> *skb,
>>>> + struct ovs_tunnel_info *out_tun_info)
>>>> +{
>>>> + struct geneve_port *geneve_port = geneve_vport(vport);
>>>> +
>>>> + if (unlikely(!OVS_CB(skb)->tun_info))
>>>> + return -EINVAL;
>>>> +
>>> Can you move this check inside ovs_vport_out_tun_info.
>>> Wenyu: Considering that the tp_src and tp_dst need be prepared before
>>> ovs_vport_get_out_tun_info() is called, we'd better do the check before
>>> doing the work to calculate tp_src and tp_dst. So I perfer to keep the
>>> check here.
>>
>> tp_src and tp_dst does not depend on out_tun_info, so the check should
>> be moved to the common function.
>>
>>>> + /*
>>>> + * Get tp_src and tp_dst, refert to geneve_build_header().
>>>> + */
>>>> + return ovs_vport_get_out_tun_info(out_tun_info,
>>>> OVS_CB(skb)->tun_info,
>>>> + ovs_dp_get_net(vport->dp),
>>>> + IPPROTO_UDP, skb->mark,
>>>> + vxlan_src_port(1, USHRT_MAX,
>>>> skb),
>>>> +
>>>> inet_sport(geneve_port->sock->sk));
>>>> +
>>>> +}
>>>> +
>>>> const struct vport_ops ovs_geneve_vport_ops = {
>>>> - .type = OVS_VPORT_TYPE_GENEVE,
>>>> - .create = geneve_tnl_create,
>>>> - .destroy = geneve_tnl_destroy,
>>>> - .get_name = geneve_get_name,
>>>> - .get_options = geneve_get_options,
>>>> - .send = geneve_send,
>>>> + .type = OVS_VPORT_TYPE_GENEVE,
>>>> + .create = geneve_tnl_create,
>>>> + .destroy = geneve_tnl_destroy,
>>>> + .get_name = geneve_get_name,
>>>> + .get_options = geneve_get_options,
>>>> + .send = geneve_send,
>>>> + .get_out_tun_info = geneve_get_out_tun_info,
>>>> };
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
