Thanks for the review. I will send out a V2 soon.
On Mon, Jul 1, 2013 at 5:06 PM, Jesse Gross <[email protected]> wrote: > On Mon, Jul 1, 2013 at 3:43 PM, Andy Zhou <[email protected]> wrote: > > diff --git a/datapath/datapath.c b/datapath/datapath.c > > index 3680391..2797e2e 100644 > > --- a/datapath/datapath.c > > +++ b/datapath/datapath.c > > @@ -1353,12 +1353,15 @@ static int ovs_flow_cmd_new_or_set(struct > sk_buff *skb, struct genl_info *info) > > */ > > error = -EEXIST; > > if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW && > > - info->nlhdr->nlmsg_flags & (NLM_F_CREATE | > NLM_F_EXCL)) > > + info->nlhdr->nlmsg_flags & (NLM_F_CREATE | > NLM_F_EXCL)) { > > + OVS_NLERR("Flow creation message rejected, > duplicate flow found.\n"); > > goto err_unlock_ovs; > > + } > > This one can happen during the normal course of operation, so I don't > think that we should log anything here. I think it should really just > be the situations where we return -EINVAL. > > > @@ -1432,6 +1437,7 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, > struct genl_info *info) > > reply = ovs_flow_cmd_build_info(flow, dp, info->snd_portid, > > info->snd_seq, OVS_FLOW_CMD_NEW); > > if (IS_ERR(reply)) { > > + OVS_NLERR("Flow get message rejected, failed to > construct the reply message.\n"); > > err = PTR_ERR(reply); > > goto unlock; > > } > > @@ -1475,6 +1481,7 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, > struct genl_info *info) > > table = ovsl_dereference(dp->table); > > flow = ovs_flow_lookup_unmasked_key(table, &match); > > if (!flow) { > > + OVS_NLERR("Flow del message rejected, flow not > found.\n"); > > err = -ENOENT; > > goto unlock; > > } > > I think it's probably not a good idea to log in these situations either. > > > diff --git a/datapath/datapath.h b/datapath/datapath.h > > index 559df69..0eb0aca 100644 > > --- a/datapath/datapath.h > > +++ b/datapath/datapath.h > > @@ -204,4 +204,8 @@ struct sk_buff *ovs_vport_cmd_build_info(struct > vport *, u32 portid, u32 seq, > > > > int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb); > > void ovs_dp_notify_wq(struct work_struct *work); > > + > > +#define OVS_NLERR(fmt, ...) \ > > + pr_info_once(fmt " NLERR: ", ##__VA_ARGS__) > > I'm not sure that this really needs to be in all caps and it could be > more descriptive. Something simple like "netlink: " would probably be > more appropriate. > > > #endif /* datapath.h */ > > diff --git a/datapath/flow.c b/datapath/flow.c > > index 2ac36b6..32fe86b 100644 > > --- a/datapath/flow.c > > +++ b/datapath/flow.c > > Many of the strings in this file basically just say that there is an > error. This does give you a pointer to the appropriate condition but > in many cases the check is more specific than just "error", so it > would be good to make the messages a little more self explanatory. > > > @@ -1289,8 +1311,10 @@ static int metadata_from_nlattrs(struct > sw_flow_match *match, u64 *attrs, > > if (*attrs & (1ULL << OVS_KEY_ATTR_SKB_MARK)) { > > uint32_t mark = nla_get_u32(a[OVS_KEY_ATTR_SKB_MARK]); > > #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,20) && > !defined(CONFIG_NETFILTER) > > - if (!is_mask && mark != 0) > > + if (!is_mask && mark != 0) { > > + OVS_NLERR("Flow metadata mark is zero.\n"); > > This error message is backwards. >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
