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@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
