On Fri, 2021-01-15 at 08:55 +0200, Jarkko Sakkinen wrote: > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: > > On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote: [...] > > > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files. > > > > Hey these interfaces were added after this patch began life. But > > looking at sysfs_emit_at() I've got to say "aah ... don't you guys > > ever read rusty's guide to interfaces?" an interface which takes in > > an absolute page position but returns a relative offset to the > > position it took in is asking for people to get it wrong. You > > should always be consistent about uses for inputs and > > outputs. Basically the only way you can ever use sysfs_emit_at in > > a show routine is as > > > > offset += sysfs_emit_at(buf, offset, ...); > > > > because you always need to track the absolute offset. > > > > It looks like we already have a couple of bugs in the kernel > > introduced by this confusion ... return sysfs_emit() vs return > > sysfs_emit_at() being the most tricky ... > > How is using sysfs_emit() different from using snprintf() for the > caller, ignoring the added safety measures? I'm new to this API. Using the sprintX variants you maintain a cursor pointer, so they all look like char *cursor = buf; ... cursor += sprintX(cursor, "...", ... ... return cursor - buf; So the input is a relative cursor and the output is the additional offset. I'm not claiming it's the best interface but it is hard to get wrong, just that if we're going to force a new interface we should make it much better. with sysfs_emit_at you use an offset "cursor" but it's hard to know without reading the function how to do it because the return is relative rather than absolute. To have an interface it would be hard to misuse, I think the best way would be to take a pointer to the offset and adjust it after use, so sysfs_emit_at(buf, &offset, ...); That way it returns void so you can't use it in place of return sysfs_emit() And you don't have to worry about whether the return is absolute or relative because it adjusts the pointer for you. The whole point about Rusty and interfaces is that if you are going to invent new interfaces you should make them easy to get right and hard to misuse. A function you can't figure out how to use until you read the source is about 2/10 on the rusty scale: https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html James
