Re: [PATCH v6 2/3] sysfs: Add a attr_is_visible function to attribute_group

[Dan Williams]

Greg KH wrote:
> On Fri, Jan 26, 2024 at 10:58:07AM -0800, Dan Williams wrote:
> > Dan Williams wrote:
> > > Greg KH wrote:
> > > [..]
> > > > > 
> > > > > Hey Greg,
> > > > > 
> > > > > I wanted to follow up on this and see if you are able to provide more
> > > > > details for reproducing or if you are able to look into it?
> > > > 
> > > > Last I tried this, it still crashed and would not boot either on my
> > > > laptop or my workstation.  I don't know how it is working properly for
> > > > you, what systems have you tried it on?
> > > > 
> > > > I'm not going to be able to look at this for many weeks due to
> > > > conference stuff, so if you want to take the series and test it and
> > > > hopefully catch my error, that would be great, I'd love to move forward
> > > > and get this merged someday.
> > > 
> > > I mentioned to Lukas that I was working on a "sysfs group visibility"
> > > patch and he pointed me to this thread. I will note that I tried to make
> > > the "hide group if all attributes are invisible" approach work, but
> > > reverted to a "new is_group_visible() callback" approach. I did read
> > > through the thread and try to improve the argument in the changelog
> > > accordingly.
> > > 
> > > I do admit to liking the cleanliness (not touching 'struct
> > > attribute_group') of the "hide if no visible attribute" approch, but see
> > > the criticism of that alternative below, and let me know if it is
> > > convincing. I tested it locally with the following hack to make the
> > > group disappear every other sysfs_update_group() event:
> > 
> > Hey Greg,
> > 
> > Ignore this version:
> > 
> > ---
> > From: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Date: Tue, 23 Jan 2024 20:20:39 -0800
> > Subject: [PATCH] sysfs: Introduce is_group_visible() for attribute_groups
> > ---
> > 
> > I am going back to your approach without a new callback, and some fixups
> > to avoid unintended directory removal. I will post that shortly with its
> > consumer.
> 
> Ignore it?  I was just about to write an email that said "maybe this is
> the right way forward" :)
> 
> What happened to cause it to not be ok? And if you can find the bug in
> the posted patch here, that would be great as well.

It was an aha moment this morning that I could rely on something like:

#define SYSFS_GROUP_INVISIBLE ((umode_t)-1)

..as the return value from either is_visible() or attr_is_visible() and
not need to define a new is_group_visible() callback.

The only downside I can see is that there is no way to know if the
is_visible() handler might return SYSFS_GROUP_INVISIBLE to decide when
to warn about deletion not finding anything to delete, or update not
finding the existing node to update.

I'll type it up and see how it looks, but if you're not worried about
the is_group_visible() addition to 'struct atttribute_group' I think
that way is less hacky than the above.