On Fri, Jul 22, 2016 at 10:26:01AM -0500, Shiraz Saleem wrote:
> On Thu, Jul 21, 2016 at 08:29:42PM +0300, Leon Romanovsky wrote:
> > 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:
> > > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > > >
> > > > > > 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.
> >
>
> We assumed the nlmsg_ API in linux/include/net/netlink.h is there for a
> purpose.
> That purpose is to normalize the return code. That API is used in places
> where
> the return code needs to be normalized, and when normalization is not needed,
> then the direct calls are used.
>
> Now since the nlm_ API in netlink.h is missing a blocking version of the
> nlmsg_unicast function, it would seem reasonable to add it there.
>
> Changing all the direct calls as you suggest would at the very least be
> less efficient since it would normalize return codes when not needed. One if with one assignment in non data path. Please look at the code. > > However, if there is a strict rule against adding an API unless you > immediately > have at least 2 callers, then I guess, we will make the direct call. The > amount > of code added will be the same, except that the next person who wants a > normalized > return code will have to duplicate the same code. Yes, we are not adding to general header file code which has not multiple callers. > > Changing other code to be less efficient so that we can meet the 2 caller > criteria > doesn't seem reasonable. I'm sorry to hear that you didn't look at the code. > >
signature.asc
Description: Digital signature
