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.
