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.
Kevin