Messages in this thread | | | Date | Wed, 20 Mar 2024 19:40:18 -0700 | From | Dan Williams <> | Subject | Re: [PATCH v2 12/14] fs/configfs: Add a callback to determine attribute visibility |
| |
Tom Lendacky wrote: > 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@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: > > 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.
The inspiration was sysfs is_visible() and an idea to reproduce the "static declaration + dynamic visibility" model that sysfs allows. However, in the near term boolean visibility is sufficient since the attributes are either on/off by vendor not read-write/read-only by vendor implementation.
| |