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

[Tom Lendacky]

On 3/13/24 16:37, Joel Becker wrote:
> On Fri, Mar 08, 2024 at 12:35:27PM -0600, 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 display. If no callback is registered, the attribute
> > is displayed by default.
>
> I'm curious what the strong value is in this extra callback.  As opposed
> to not generating the attribute in the absence of a TPM (why create a
> config_item at all?), merely having an empty response from the attribute,
> or having `->show()` return -ENODEV or similar.

The value is to reduce the complexity of registering with the TSM support 
across multiple vendors. There is a base set of attributes that are common 
across vendors and some that are specific to vendors. Creating this 
structure in the TSM support can get unwieldy. This would make it simple 
to determine if support is provided since the attribute will either be 
present or not.

This would also make the support similar to sysfs in the ability to 
dynamically hide or show attributes.

> > Cc: Joel Becker <jlbec@xxxxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > ---
> >   fs/configfs/file.c       |  13 +++++
> >   include/linux/configfs.h | 114 +++++++++++++++++++++++++++------------
> >   2 files changed, 93 insertions(+), 34 deletions(-)
> >
> > diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> > index 0ad32150611e..c758bcc11235 100644
> > --- a/fs/configfs/file.c
> > +++ b/fs/configfs/file.c
> > @@ -451,6 +451,12 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib
> >   	umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
> >   	int error = 0;
> >   
> > +	if (attr->ca_is_visible) {
> > +		mode = attr->ca_is_visible(item, attr);
> > +		if (!mode)
> > +			return 0;
>
> What value do we get from carrying the mode through here?  The API
> proposed is "visible or not", which is a boolean.  Overloading that with
> "also set the mode" is confusing, and it also can lead to the divergent
> codepath problem you mentioned in your response, where
> `->ca_is_visible()` fails to return the mode correctly.  If this was simpl
> a boolean hook, the code could read like so:

A boolean would work. There was a request to make this similar to the 
sysfs attribute visibility. I certainly can make this a simple bool 
function if that is preferable.

> ```
> 	umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
> 	int error = 0;
>
> 	if (attr->ca_is_visible && !attr->ca_is_visible(item, attr))
>     		return 0;
> ```
>
> > diff --git a/include/linux/configfs.h b/include/linux/configfs.h
> > index 2606711adb18..18011f78ffde 100644
> > --- a/include/linux/configfs.h
> > +++ b/include/linux/configfs.h
> > @@ -112,39 +112,64 @@ static inline void configfs_add_default_group(struct config_group *new_group,
> >   	list_add_tail(&new_group->group_entry, &group->default_groups);
> >   }
> >   
> > +typedef umode_t (*configfs_is_visible_t)(const struct config_item *item,
> > +					 const struct configfs_attribute *attr);
> > +
>
> We don't use typedefs of op functions anywhere else in configfs or
> frankly the entire filesystem API.  Adding one here would just introduce
> confusion.

Sure, I can remove that and do something similar to show/store.

Thanks,
Tom

> >   struct configfs_attribute {
> >   	const char		*ca_name;
> >   	struct module 		*ca_owner;
> >   	umode_t			ca_mode;
> > +	configfs_is_visible_t	ca_is_visible;
> >   	ssize_t (*show)(struct config_item *, char *);
> >   	ssize_t (*store)(struct config_item *, const char *, size_t);
> >   };
> >   
>
> Thanks,
> Joel