Dan Williams wrote:
Dan Williams wrote:
Lukas Wunner wrote:
On Sat, Apr 27, 2024 at 09:49:41AM -0700, Dan Williams wrote:
Lukas Wunner wrote:
But I want to raise awareness that the inability to hide empty attribute groups feels awkward.
That is fair, it was definitely some gymnastics to only change user visible behavior for new "invisible aware" attribute groups that opt-in while leaving all the legacy cases alone.
The concern is knowing when it is ok to call an is_visible() callback with a NULL @attr argument, or knowing when an empty array actually means "hide the group directory".
We could add a sentinel value to indicate "I am an empty attribute list *AND* I want my directory hidden by default". However, that's almost identical to requiring a placeholder attribute in the list just to make __first_visible() happy.
Other ideas?
Perhaps an optional ->is_group_visible() callback in struct attribute_group which gets passed only the struct kobject pointer?
At least for PCI device authentication, that would be sufficient. I could get from the kobject to the corresponding struct device, then determine whether the device supports authentication or not.
Because it's a new, optional callback, there should be no compatibility issues. The SYSFS_GROUP_INVISIBLE return code from the ->is_visible() call for individual attributes would not be needed then, at least in my use case.
That's where I started with this, but decided it was overkill to increase the size of that data structure globally for a small number of use cases.
Perhaps could try something like this:
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c index d22ad67a0f32..f4054cf08e58 100644 --- a/fs/sysfs/group.c +++ b/fs/sysfs/group.c @@ -33,11 +33,23 @@ static void remove_files(struct kernfs_node *parent,
static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj) {
if (grp->attrs && grp->attrs[0] && grp->is_visible)return grp->is_visible(kobj, grp->attrs[0], 0);
if (grp->attrs && grp->is_visible) {struct attribute *attr = grp->attrs[0];struct attribute empty_attr = { 0 };
if (grp->bin_attrs && grp->bin_attrs[0] && grp->is_bin_visible)return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
if (!attr)attr = &empty_attr;return grp->is_visible(kobj, attr, 0);}if (grp->bin_attrs && grp->is_bin_visible) {struct bin_attribute *bin_attr = grp->bin_attrs[0];struct bin_attribute empty_bin_attr = { 0 };if (!bin_attr)bin_attr = &empty_bin_attr;return grp->is_bin_visible(kobj, bin_attr, 0);} return 0;}
...because it is highly likely that existing is_visible() callers will return @attr->mode when they do not recognize the attribute. But this could lead to some subtle bugs if something only checks the attribute index value. For example:
lbr_is_visible(struct kobject *kobj, struct attribute *attr, int i) { /* branches */ if (i == 0) return x86_pmu.lbr_nr ? attr->mode : 0;
return (x86_pmu.flags & PMU_FL_BR_CNTR) ? attr->mode : 0;}
...but in this case we get lucky because it would return attr->mode which is 0.
Oh, but if an is_visible() callback gets confused by empty_attr, that's detectable:
mode = grp->is_visible(kobj, attr, 0); if ((mode & ~SYSFS_GROUP_INVISIBLE) && attr == empty_attr) return 0;
...i.e. the only acceptable responses to an empty_attr check is 0 or ~SYSFS_GROUP_INVISIBLE.