[ovs-dev] [PATCH] datapath: add netlink error message to help kernel userspace integration.

[Andy Zhou]

Thanks for the review. I will send out a V2 soon.


On Mon, Jul 1, 2013 at 5:06 PM, Jesse Gross <jesse at nicira.com> wrote:

> On Mon, Jul 1, 2013 at 3:43 PM, Andy Zhou <azhou at nicira.com> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130701/570f8ffe/attachment-0003.html>