Re: [PATCH V2] Add flow control to the portmapper

On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:

On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:

quoted

On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:

quoted

On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:

quoted

You are the one user of this new inline function.
Why don't you directly call to netlink_unicast() in your ibnl_unicast()
without messing with widely visible header file?

Since there is a non-blocking version of nlmsg_unicast(), the idea is 
to make a blocking version available to others as well as maintain 
consistency of existing code.

In such way, please provide patch series which will convert all other
users to this new call.

➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);

These usages of netlink_unicast() with blocking are not the same as the new
nlmsg_unicast_block() function. 

Really?
Did you look in the code?
Let's take first function from that grep output

414         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
415         if (err < 0) {
			... do something ...
437         } else
			... do something else ...

which fits nicely with your proposal.

+static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32 portid)
+{
+       int err;
+
+       err = netlink_unicast(sk, skb, portid, 0);
+       if (err > 0)
+               err = 0;
+
+       return err;
+}

You can't drop in nlmsg_unicast_block() in 
place of netlink_unicast() in these places. I'm not going to introduce code 
which modifies old behavior.

Again, you aren't changing any behaviour.
Anyway we are not adding general function to common include file just
because one caller wants it.