Re: [PATCH v4 12/15] fs/configfs: Add a callback to determine attribute visibility

[Tom Lendacky]

On 4/26/24 16:48, Dan Williams wrote:
> Tom Lendacky wrote:
> > In order to support dynamic decisions as to whether an attribute should be
> > created, add a callback that returns a bool to indicate whether the
> > attribute should be displayed. If no callback is registered, the attribute
> > is displayed by default.
> >
> > Cc: Joel Becker <jlbec@xxxxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Co-developed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > ---
> >   fs/configfs/dir.c        | 20 ++++++++++++++++++++
> >   include/linux/configfs.h |  3 +++
> >   2 files changed, 23 insertions(+)
> >
> > diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> > index 18677cd4e62f..463e66258507 100644
> > --- a/fs/configfs/dir.c
> > +++ b/fs/configfs/dir.c
> > @@ -580,6 +580,7 @@ static void detach_attrs(struct config_item * item)
> >   static int populate_attrs(struct config_item *item)
> >   {
> >   	const struct config_item_type *t = item->ci_type;
> > +	struct configfs_group_operations *ops;
> >   	struct configfs_attribute *attr;
> >   	struct configfs_bin_attribute *bin_attr;
> >   	int error = 0;
> > @@ -587,14 +588,33 @@ static int populate_attrs(struct config_item *item)
> >   
> >   	if (!t)
> >   		return -EINVAL;
> > +
> > +	ops = t->ct_group_ops;
> > +	if (!ops) {
> > +		struct config_group *g = item->ci_group;
> > +
> > +		/*
> > +		 * No item specific group operations, check if the item's group
> > +		 * has group operations.
> > +		 */
> > +		if (g && g->cg_item.ci_type)
> > +			ops = g->cg_item.ci_type->ct_group_ops;
>
> Oh, I would not have expected to need to consider any alternate group
> ops for attribute visibility beyond t->ct_group_ops. However in my RFC
> example I made this mistake:

Right, that's the question. Do you use the default group ops that are 
created for TSM report or do you require the ops be part of the 
attributes. It is definitely easy to do the latter.

>    static struct configfs_group_operations tsm_report_group_ops = {
>           .make_item = tsm_report_make_item,
> +        .is_visible = tsm_report_attr_visible,
> +        .is_bin_visible = tsm_report_bin_attr_visible,
>    };
>   
> Which in retrospect is the wrong level, and I suspect only reachable if
> you do the the above awkward indirection ("ops =
> g->cg_item.ci_type->ct_group_ops"). Instead, I was expecting symmetry
> with sysfs where the object that carries ->attrs also carries
> ->is_visible, so something like this:
>
> + static struct configfs_group_operations tsm_report_attr_group_ops = {
> +         .is_visible = tsm_report_attr_visible,
> +         .is_bin_visible = tsm_report_bin_attr_visible,
> + };
>
>    const struct config_item_type tsm_report_type = {
>            .ct_owner = THIS_MODULE,
>            .ct_bin_attrs = tsm_report_bin_attrs,
>            .ct_attrs = tsm_report_attrs,
>            .ct_item_ops = &tsm_report_item_ops,
> +         .ct_group_ops = &tsm_report_attr_group_ops
>    };
>    EXPORT_SYMBOL_GPL(tsm_report_default_type);
>
> ..because is_visible() at the g->cg_item.ci_type->ct_group_ops level
> would seem to mean parent directory visibility which is mismatched.
>
> However as I stare at this a bit more it sinks in that configfs "group"
> != sysfs "group". So I am open to the suggestion that ci_item_ops is the
> right place to house item attribute visibility callbacks, or even a new
> "ci_attr_ops" expressly for this purpose. Either way my expectation is
> that config_item_type can get to the visibilty callbacks for its
> attributes without needing to traverse any other groups or items.

I'll update the patchset to do that.

Thanks,
Tom

>