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 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.
Thanks
>
> Jason
> --
> 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
