On Wed, May 05, 2021 at 02:29:16PM -0300, Jason Gunthorpe wrote:
> On Wed, May 05, 2021 at 06:26:06PM +0200, Greg KH wrote:
> > > They are in many places, for instance.
> > >
> > > int device_create_file(struct device *dev,
> > > const struct device_attribute *attr)
> > >
> > > We loose the type safety when working with attribute arrays, and
> > > people can just bypass the "proper" APIs to raw sysfs ones whenever
> > > they like.
> > >
> > > It is fundamentally completely wrong to attach a 'struct
> > > kobject_attribute' to a 'struct device' kobject.
> >
> > But it works because we are using C and we don't have RTTI :)
> >
> > Yes, it's horrid, but we do it because we "know" the real type that is
> > being called here. That was an explicit design decision at the time.
>
> I think it is beyond horrid. Just so everyone is clear on what is
> happening here..
>
> RDMA has this:
>
> struct hw_stats_attribute {
> struct attribute attr;
> ssize_t (*show)(struct kobject *kobj,
> struct attribute *attr, char *buf);
>
> And it has two kobject types, a struct device kobject and a ib_port
> kobject.
>
> When the user invokes show on the struct device sysfs we have this
> call path:
>
> dev_sysfs_ops
> dev_attr_show()
> struct device_attribute *dev_attr = to_dev_attr(attr);
> ret = dev_attr->show(dev, dev_attr, buf);
> show_hw_stats()
> struct hw_stats_attribute *hsa = container_of(attr, struct hw_stats_attribute, attr)
>
> And from the ib_port kobject we have this one:
>
> port_sysfs_ops
> port_attr_show()
> struct port_attribute *port_attr =
> container_of(attr, struct port_attribute, attr);
> return port_attr->show(p, port_attr, buf);
> show_hw_stats()
> struct hw_stats_attribute *hsa = container_of(attr, struct hw_stats_attribute, attr)
>
> Then show_hw_stats() goes on to detect which call chain it uses so it
> can apply the proper container of to the kobj:
Wait, what? That's not how any of this was designed, you should not be
"sharing" a callback of different types of objects, because:
>
> if (!hsa->port_num)
> dev = container_of((struct device *)kobj,
> struct ib_device, dev);
> else
> port = container_of(kobj, struct ib_port, kobj);
Yeah, ick.
No, that's not how this was designed or intended to be used. Why not
just have 2 different show functions?
thanks,
greg k-h
