On Mon, Nov 7, 2011 at 1:35 PM, Jesse Gross <[email protected]> wrote:
> On Mon, Nov 7, 2011 at 12:49 PM, Pravin B Shelar <[email protected]> wrote:
>> Most of issue are reported by checkpatch.pl
>>
>> Signed-off-by: Pravin B Shelar <[email protected]>
>> 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.
Yes, it is minimum set.
I am not sure why do we need to include header even if it is included
indirectly.
>
>> 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.
ok.
>
>> @@ -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.
>
ok
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
