On Mon, Dec 05, 2022 at 12:58:52PM +0100, Kevin Brodsky wrote:
On 05/12/2022 11:15, Tudor Cretu wrote:
Note that depicting number of arguments to be fetched from va_list based on provided command will not solve the problem as some (like KEYCTL_SEARCH) still allow optional ones.
This change looks good to me as-is already and I'm happy with it. I am curious though about what do you mean by some provided commands allow optional varargs. Looking at the man page for keyctl, it seems that for each command it states which arguments are ignored if any. The way I understand it is that if it's not mentioned that an argument is ignored, then it might be used and it's expected to be provided. Also, reading the man page for KEYCTL_SEARCH, it looks to me that all the args are required, even arg5 must be intentionally provided as a zero or nonzero value, so I'm a bit confused by this.
Also, looking at the implementation of the keyctl syscall in Linux, for each command option, the kernel expects a fixed number of arguments provided, which seems to match the man page. So, in my opinion, you could still go for depicting the number of arguments based on provided command, but I'm not sure it's worth the effort really, and I'm very happy with the current change as well.
Right,so kernel does not have much choice here but to handle all 'expected' arguments. The man pages indeed mention 4th argument though it can remain at a default value and this fact is being leveraged by keyutils (https://github.com/Distrotech/keyutils/blob/master/keyctl.c#L663), while libkeyutils does mandate explicitly providing one. So overall I think I got too wrapped up when looking at keyutils and indeed we can get a lengthy switch there. I do not mind the change if that is what we want to do. Thanks for being so scrupulous!
Thanks for the clarifications, it makes sense to me now. I don't have any preference related to having the switch case or not, so we can leave it like this from my point of view.
Seconded. In my view this approach is as good as it gets, the diff is minimal and gets rid of the variadic wrapper altogether, which is the best thing to do with variadic functions! I'd just remove that paragraph in the commit message; even in the case where having a big switch would technically be possible, there would really be no reason to prefer that approach.
The reason why the 'switch' is even being considered here is that the upstream maintainers suggested similar approach for semctl although that's because of how that one is already being handled in glibc/musl (https://lore.kernel.org/ltp/87mt8at3md.fsf@suse.de/). keyctl is bit different here as it's not being provided by either. I do not really have a strong preference here: having macro wrapper is convenient as it deals wit all, having a switch means each caller needs to explicitly provide a default value when needed otherwise we are back to undefined behaviour. Note that even man is listing things like: /* * For interest, get the ID of the authorization key and * display it. */ auth_key = keyctl(KEYCTL_GET_KEYRING_ID, KEY_SPEC_REQKEY_AUTH_KEY);
where KEYCTL_GET_KEYRING_ID can/should take 2 arguments ....
So indeed having the macro wrapper is both more efficient and safer in the end. FWIW: it seems that current version was 'inspired' by: https://github.com/Distrotech/keyutils/blob/master/keyutils.c#L66
To wrap things up: I'm happy to stay with v2.
--- BR B.
Kevin