Re: [rdma-next v1 08/33] RDMA/netlink: Simplify the put_msg and put_attr

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3787 bytes --]

On Wed, Aug 09, 2017 at 01:00:21PM -0400, Doug Ledford wrote:
> On 8/9/2017 12:03 PM, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > Reuse standard macros to cancel the netlink message
> > in case of error.
> >
> > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Reviewed-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> > ---
> >  drivers/infiniband/core/netlink.c | 31 +++++++++----------------------
> >  1 file changed, 9 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
> > index c5ee62a24960..fc8b45d4140a 100644
> > --- a/drivers/infiniband/core/netlink.c
> > +++ b/drivers/infiniband/core/netlink.c
> > @@ -126,36 +126,23 @@ EXPORT_SYMBOL(rdma_nl_unregister);
> >  void *ibnl_put_msg(struct sk_buff *skb, struct nlmsghdr **nlh, int seq,
> >  		   int len, int client, int op, int flags)
> >  {
> > -	unsigned char *prev_tail;
> > -
> > -	prev_tail = skb_tail_pointer(skb);
> > -	*nlh = nlmsg_put(skb, 0, seq, RDMA_NL_GET_TYPE(client, op),
> > -			 len, flags);
> > -	if (!*nlh)
> > -		goto out_nlmsg_trim;
> > -	(*nlh)->nlmsg_len = skb_tail_pointer(skb) - prev_tail;
> > +	*nlh = nlmsg_put(skb, 0, seq, RDMA_NL_GET_TYPE(client, op), len, flags);
> > +	if (!*nlh) {
> > +		nlmsg_cancel(skb, *nlh);
>                                   ^^^^ This looks funny.  It's
> technically correct, but it looks like an error because you are using a
> value that we just checked for an error, saw an error, and then use it
> anwyay.  I would prefer that be NULL instead just like you write return
> NULL just below.

You are right from the machine-perspective, it is correct code, because
nlmxg_cancel is actually calling to nlmg_trim, which checks the provided
argument (*nlh) for NULL and skips it. But from the code reviewer, it is
better to just remove this nlmsg_cancel line because it does nothing.

>
> > +		return NULL;
> > +	}
> >  	return nlmsg_data(*nlh);
> > -
> > -out_nlmsg_trim:
> > -	nlmsg_trim(skb, prev_tail);
>
> In the new code, on error, you cancel the message outright, while the
> old code here does a nlmsg_trim().  Isn't this a functional change in
> addition to just switching to using the standard helpers?  Same question
> for below...

nlmsg_cancel calls unconditionally to nlmsg_trim and the code was
restructured in such way that nlmsg_trim is always executed in these
flows, so no actual functional change.

 540 static inline void nlmsg_trim(struct sk_buff *skb, const void *mark)
 541 {
 542         if (mark) {
 543                 WARN_ON((unsigned char *) mark < skb->data);
 544                 skb_trim(skb, (unsigned char *) mark - skb->data);
 545         }
 546 }

 <...>

 556 static inline void nlmsg_cancel(struct sk_buff *skb, struct nlmsghdr *nlh)
 557 {
 558         nlmsg_trim(skb, nlh);
 559 }


>
> > -	return NULL;
> >  }
> >  EXPORT_SYMBOL(ibnl_put_msg);
> >
> >  int ibnl_put_attr(struct sk_buff *skb, struct nlmsghdr *nlh,
> >  		  int len, void *data, int type)
> >  {
> > -	unsigned char *prev_tail;
> > -
> > -	prev_tail = skb_tail_pointer(skb);
> > -	if (nla_put(skb, type, len, data))
> > -		goto nla_put_failure;
> > -	nlh->nlmsg_len += skb_tail_pointer(skb) - prev_tail;
> > +	if (nla_put(skb, type, len, data)) {
> > +		nlmsg_cancel(skb, nlh);
> > +		return -EMSGSIZE;
> > +	}
> >  	return 0;
> > -
> > -nla_put_failure:
> > -	nlmsg_trim(skb, prev_tail - nlh->nlmsg_len);
> > -	return -EMSGSIZE;
> >  }
> >  EXPORT_SYMBOL(ibnl_put_attr);
> >
> >
>
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]