On 01/17/2015 02:09 PM, Vivien Didelot wrote:
Hi Guenter, Greg,
[ .. ]
BTW Guenter, does this patch make sense to you?
It does make sense to me to only use the return value from is_visible for the mode. As for which bits to use, I am not entirely sure. I think it would be more important to first decide which bits should be acceptable to start with. Then I would _always_ only use the bits from mode, masked against the valid bits, whatever they are. umode_t mode = (*attr)->mode; ... if (grp->is_visible) { mode = grp->is_visible(kobj, *attr, i); if (!mode) continue; } WARN(mode & ~(S_IRUGO | S_IWUGO | SYSFS_PREALLOC), /* optional */ "Attribute %s: Invalid permission 0x%x\n", (*attr)->name, mode); mode &= S_IRUGO | S_IWUGO | SYSFS_PREALLOC; error = sysfs_add_file_mode_ns(parent, *attr, false, mode, NULL); ...
My assumption here was that the attribute group is_visible function should just be able to adjust the UGO bits. Am I correct?
I would think so.
I'm not even sure about the execute permission though. Only one driver uses it for an attribute and it seems wrong, in drivers/hid/hid-lg4ff.c: static DEVICE_ATTR(range, S_IRWXU | S_IRWXG | S_IROTH, lg4ff_range_show, lg4ff_range_store);
That seems wrong.
The actual behavior seems wrong to me. Again, what happens is you return SYSFS_PREALLOC, that the underlying sysfs_add_file_mode_ns() function is actually checking?
Ultimately, the implementor asked for it.
IMHO, if we want an attribute group to only be able to "hide or show" an attribute, then is_visible (as the name suggests) should return a boolean. If we want it be able to adjust permissions (as it seems correct, given the examples), we should identify which permissions are OK to change, deprecate is_visible function (to avoid code break) in favor of a new one which limits the bits to that scope.
Up to Greg to decide. From my perspective, we have lived with is_visible for several years and overall it seems to work. Sure, it lacks a clear API, but that can be fixed without changing a lot of code just to replace the function name. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/