Re: [PATCH rdma-next 1/5] RDMA/netlink: Remove netlink clients infrastructure

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

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

On Thu, Jun 08, 2017 at 08:29:25AM -0700, Bart Van Assche wrote:
> On 06/08/17 07:38, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > RDMA netlink has complicated infrastructure to add and remove netlink
> > clients to NETLINK_RDMA family. This complicates the code and not in
> > use because not many clients are available (3 clients) and most of them
> > (2 clients) are statically compiled together with netlink.c.
> >
> > The following patch refactors RDMA netlink and opens door for the future
> > patches which will be able to get rid of a lot of dead iwcm* code.
>
> Hello Leon,
>
> There are multiple changes in this patch:
> - Renaming ibnl_add_client() and ibnl_remove_client() into
> rdma_nl_register() and rdma_nl_unregister().
> - Removal of the module pointers from ibnl_ls_cb_table[].
> - Conversion of client_list from a linked list into an array
> (rdma_nl_types[]).
>
> Could this patch have been split?

I tried, but once I got rid of the client (delete function), i was
required to change the function signature and while I was there, I changed
the name. So if it is possible, I would like to have one patch and not
many patches which changes the same lines of code.

>
> > +static bool is_nl_msg_valid(unsigned int type, unsigned int op)
> >  {
> > -	[ ... ]
> > +	unsigned int max_num_ops;
> > +
> > +	switch (type) {
> > +	case RDMA_NL_RDMA_CM:
> > +		max_num_ops = RDMA_NL_RDMA_CM_NUM_OPS;
> > +		break;
> > +	case RDMA_NL_IWCM:
> > +		max_num_ops = RDMA_NL_IWPM_NUM_OPS;
> > +		break;
> > +	case RDMA_NL_LS:
> > +		max_num_ops = RDMA_NL_LS_NUM_OPS;
> > +		break;
> > +	case RDMA_NL_RSVD:
> > +	case RDMA_NL_I40IW:
> > +	default:
> > +		return false;
> >  	}
> > +	return (op < max_num_ops) ? true : false;
> > +}
>
> Can this code be made more compact by storing the *_NUM_OPS constants in
> an array?

I thought that it is more easy to read, but have no strong opinion about
it.

>
> > +void rdma_nl_register(unsigned int index,
> > +		      const struct ibnl_client_cbs cb_table[])
> >  {
> > -	[ ... ]
> > -	return -EINVAL;
> > +	rdma_nl_types[index].cb_table = cb_table;
> > +	mutex_unlock(&rdma_nl_mutex);
> > +}
> > +EXPORT_SYMBOL(rdma_nl_register);
>
> Shouldn't this function return an error code or print a warning message
> if rdma_nl_types[index].cb_table had already been set?

Agree, warning will be good there. I don't want to return value from
this function, because callers are not really interested in
success/failure once they finished to debug their code.

>
> Otherwise this patch looks like a welcome cleanup and simplification to me.

Thanks for taking time to look on these patches.

>
> Bart.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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