On Monday, 9 December 2024 14:24:36 CST Arnd Bergmann wrote: > On Mon, Dec 9, 2024, at 19:58, Elizabeth Figura wrote: > > == Previous versions == > > > > No changes were made from v5 other than rebasing on top of the 6.13-rc1 > > char-misc-next tree. > > > > I would like to repeat a question from the last round of review, though. Two > > changes were suggested related to API design, which I did not make because the > > APIs in question were already released in upstream Linux. However, the driver is > > also completely nonfunctional and hidden behind BROKEN, so would this be > > acceptable anyway? The changes in question are: > > If it was impossible to use the driver, there is no regression. > I feel the entire point of marking it as broken was to be able > to add that type of change. Makes sense. [I figured that the BROKEN was just there to prevent anyone from trying to use a half-finished driver, and the point of committing a half-finished driver was just to reduce the number of patches that needed to be resent.] I'll make these changes and resend. > > * rename NTSYNC_IOC_SEM_POST to NTSYNC_IOC_SEM_RELEASE (matching the NT > > terminology instead of POSIX), > > No objections my me on either name. > > > * change object creation ioctls to return the fds directly in the return value > > instead of through the args struct. I would also still appreciate a > > clarification on the advice in [1], which is why I didn't do this in the first > > place. > > > > [1] https://docs.kernel.org/driver-api/ioctl.html#return-code > > The git log tells me that I have written that, but I don't remember > why I put that in, maybe someone else suggested it. > > My feeling right now is that returning a file descriptor number > as a small positive integer from the ioctl() return code makes > sense. On the other hand, returning pointers, negative signed > integers or large (> 32bit) 'unsigned long' values can cause > a number of issues, so I would avoid all those the same way we > discourage passing those integers as a literal 'arg' into ioctl() > instead of going through a pointer. Ah, that makes sense to me, thanks.
