[ovs-dev] [PATCH] datapath: Fix coding style issues.

[Jesse Gross]

On Mon, Nov 7, 2011 at 12:49 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> Most of issue are reported by checkpatch.pl
>
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
> Bug #7771

I have a couple of questions but otherwise this looks very thorough to me.

> diff --git a/datapath/actions.c b/datapath/actions.c
> index b5b92ba..daa8c6b 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -10,18 +10,8 @@
>
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> -#include <linux/skbuff.h>
>  #include <linux/in.h>
> -#include <linux/ip.h>
>  #include <linux/openvswitch.h>
> -#include <linux/tcp.h>
> -#include <linux/udp.h>
> -#include <linux/in6.h>
> -#include <linux/if_arp.h>
> -#include <linux/if_vlan.h>
> -#include <net/inet_ecn.h>
> -#include <net/ip.h>
> -#include <net/checksum.h>

You remove a lot of included header files throughout this patch.  How
did you come up with the list?  It seems like the minimum set but
usually the rule is that if you use something directly then you should
include the header file even if something else that you use also
happens to include it.

> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 87056cf..8a530a7 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
>  static int validate_userspace(const struct nlattr *attr)
>  {
> -       static const struct nla_policy userspace_policy[OVS_USERSPACE_ATTR_MAX + 1] =
> -       {
> +       static const struct nla_policy
> +       userspace_policy[OVS_USERSPACE_ATTR_MAX + 1] =  {

I think it's probably better to keep this on one line, even if it
makes it a long one.

> @@ -1051,11 +1037,12 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
>                flow_tbl_insert(table, flow);
>
>                reply = ovs_flow_cmd_build_info(flow, dp, info->snd_pid,
> -                                               info->snd_seq, OVS_FLOW_CMD_NEW);
> +                                               info->snd_seq,
> +                                               OVS_FLOW_CMD_NEW);
>        } else {
>                /* We found a matching flow. */
>                struct sw_flow_actions *old_acts;
> -
> +               struct nlattr *acts_attrs;
>                /* Bail out if we're not allowed to modify an existing flow.
>                 * We accept NLM_F_CREATE in place of the intended NLM_F_EXCL
>                 * because Generic Netlink treats the latter as a dump

Can you add a blank line before this comment like there was before?
It makes it a little easier to read.