On 18/06/26 21:14, Andy Shevchenko wrote: > On Thu, Jun 18, 2026 at 05:14:19PM +0100, Rodrigo Alencar wrote: > > On 18/06/26 16:06, Nuno Sá wrote: > > > On Thu, Jun 18, 2026 at 02:27:22PM +0100, Rodrigo Alencar via B4 Relay > > > wrote: > > ... > > > > > + dev_attr->attr.name = kasprintf(GFP_KERNEL, "%s%s", prefix, > > > > postfix); > > > > + if (!dev_attr->attr.name) > > > > return -ENOMEM; > > > > > > I don't oppose the change. Looks like a nice cleanup. > > May I oppose it? I found use scnprintf() is harder to follow in comparison to > nice kasprintf() that takes care for the dynamically allocated buffer.
In the next patch the function is reused in a sysfs attribute read handler, a context wich would not be nice to have dynamic allocation. vscnprintf() is the main building block of sysfs_emit() which limits the buffer length to a page size, so I used scnprintf() trying not to deviate much from that. kasprintf() it is still used in the caller, where the logic was a bit confusing as it tried to avoid multiple allocations. > Also there is a chance to get a name silently cut due to insufficient space. > Besides that this function can't be used (again due to 'c') in > kasprintf()-like > wrapper. I do not consider this as a good approach. Have you looked at seq_buf > instead? NAME_MAX is not the maximum length a filename can have? I suppose there should be enough space for the channel-prefix. Indeed, seq_buf can be used and it cleans up things a bit as it tracks the the position in the buffer. > > > > But bear in mind this very sensible as any subtle mistake means ABI > > > breakage. > > Which immediately raises a question of test coverage. Do we have one? If not, > this code must be accompanied with one. Agreed. Will see to have tests for v7. > > Yes! I tried to be careful... this is dangerous stuff! -- Kind regards, Rodrigo Alencar
