> On Wed, Feb 28, 2018 at 10:33:46AM -0700, Jason Gunthorpe wrote:
> > On Wed, Feb 28, 2018 at 10:38:35AM -0600, Steve Wise wrote:
> > >
> > > > > > > > Steve,
> > > > > > > >
> > > > > > > > I imagined such function callback code a little bit simpler
than
> > > it is
> > > > > > > now:
> > > > > > > >
> > > > > > > > nldev_res_get_XXX_dumpit {
> > > > > > > > initialization
> > > > > > > > checks
> > > > > > > >
> > > > > > > > ->callback(skb, RDMA_RESTRACK_XXX)
> > > > > > > >
> > > > > > > > close/error handling
> > > > > > > > }
> > > > > > > >
> > > > > > > > It will simplify the callback table a bit.
> > > > > > >
> > > > > > > That would work, at the expense of duplicating the parsing of
the nl
> > > req
> > > > and
> > > > > > > building of the nl reply table. I'll look into it. It might
make
> > > things a
> > > > > > > little cleaner.
> > > > > >
> > > > > > They can be static internal helpers, so no actual duplication
will be.
> > > > >
> > > > > And after more thinking, it seems that you don't need callback
too,
> > > > > simple wrapper function will be enough (everything is declared in
one
> > > > > file, no need to dynamically change function callback, export it
to
> > > > > other modules e.t.c).
> > > > >
> > > >
> > > > So you propose, I think:
> > > >
> > > > remove the logic in res_get_common_dumpit() that requires passing it
the
> > > > nldev_cmd and nldev_attr values. Move these to helper functions and
call
> > > them
> > > > directly from the nldev_res_get_XXX_dumpit() functions. This
allows
> > > removing
> > > > the nldev_fill_res_entry struct entirely.
> > > >
> > > > But we still need to either pass the resource-specific fill function
> > > pointer to
> > > > res_get_common_dumpit(), or have a helper function that switches on
> > > res_type
> > > > and calls the appropriate fill function. Are you saying you
propose the
> > > latter vs
> > > > passing the function ptr?
> > > >
> > >
> > > I'm inclined to leave the design as I have done it. I'm not sure
having
> > > each nldev_res_get_XXX_dumpit() function do something like below makes
it
> > > easier to read or more maintainable:
> >
> > I like this version with the fill callback, makes a lot of sense to
> > me. Having only fill vary per type seems good with the way things are
> > now.
> >
> > But not sure what picture Leon has to compare it with..
> >
> > BTW:
> >
> > +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = {
> > + [RDMA_RESTRACK_QP] = {
> >
> > Missing 'const'
>
> I don't like this construction:
> +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = {
> + [RDMA_RESTRACK_QP] = {
> + .fill_res_func = fill_res_qp_entry,
> + .res_type = RDMA_RESTRACK_QP,
> + .nldev_cmd = RDMA_NLDEV_CMD_RES_QP_GET,
> + .nldev_attr = RDMA_NLDEV_ATTR_RES_QP,
> + },
> +};
>
> Constant and duplicated fields:
> res_type == index == RDMA_RESTRACK_QP
> nldev_cmd, nldev_attr
>
I could remove res_type, and pass it to res_get_common_dumpit() vs passing
the fill_res_entry struct ptr.
> I had in mind very simple picture:
>
> static check_input(...)
> { ... }
> static fill_entries(..)
> { ... }
>
> static qp_dummpit_func(..)
> {
> check_input(QP, ...)
> fill_entries(QP, ...)
> return ..;
> }
> static pd_dummpit_func(..)
> {
> check_input(PD, ...)
> fill_entries(PD, ...)
> return ..;
> }
>
> e.t.c.
>
> Please don't over-complicate things where it is not necessary, all those
> objects with dump functions will be constant and we won't add many objects
> in near future.
>
Your proposed check_input() would have to take res_type, nldev_cmd, and
nldev_attr, correct? I don't want to respin this to have you shoot it down
again because I'm passing too many parameters...
--
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
