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

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2783 bytes --]

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.

> +		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...

> -	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: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]