Messages in this thread Patch in this message | | | Date | Wed, 13 Mar 2024 14:37:17 -0700 | From | Joel Becker <> | Subject | Re: [PATCH v2 12/14] fs/configfs: Add a callback to determine attribute visibility |
| |
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.
> > Cc: Joel Becker <jlbec@evilplan.org> > Cc: Christoph Hellwig <hch@lst.de> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > 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:
``` 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.
> 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
--
Life's Little Instruction Book #306
"Take a nap on Sunday afternoons."
http://www.jlbec.org/ jlbec@evilplan.org
| |