On Mon, Dec 18, 2023 at 8:34 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Mon, Dec 18, 2023 at 7:58 PM Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxx> wrote: > > > > On Mon, 18 Dec 2023 at 17:48, Alexei Starovoitov > > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > > > > > > Point taken. > > > We can do s/__u32 token_fd/__u64 token/ > > > and waste upper 32-bit as flags that indicate that lower 32-bit is an FD > > > or > > > are you ok with __u32 token that is 'fd + 1'. > > > > No, you make it follow the standard pattern that Unix has always had: > > file descriptors are _signed_ integer, and negative means error (or > > special cases). > > > > Now, traditionally a 'fd' is literally just of type "int", but for > > structures it's actually good to make it be a sized entity, so just > > make it be __s32, and make any special cases be actual negative > > numbers. > > > > Because I'll just go out on a limb and say that two billion file > > descriptors is enough for anybody, and if we ever were to hit that > > number, we'll have *way* more serious problems elsewhere long long > > before. And in practice, "int" is 32-bit on all current and > > near-future architectures, so "__s32" really is the same as "int" in > > all practical respects, and making the size explicit is just a good > > idea. > > > > You might want to perhaps pre-reserve a few negative numbers for > > actual special cases, eg "openat()" uses > > > > #define AT_FDCWD -100 > > > > which I don't think is a great example to follow in the details: it > > should have parenthesis, and "100" is a rather odd number to choose, > > but it's certainly an example of a not-fundamentally-broken "not a > > file descriptor, but a special case". > > > > Now, if you have a 'flags' or 'cmd' field for *other* reasons, then > > you can certainly just use one of the flags for "I have a file > > descriptor". But don't do some odd "translate values", and don't add > > 32 bits just for that. > > > > Makes sense. Yes, we do have flags for all commands accepting token > FD, except for one, BPF_BTF_LOAD, but it's trivial to add flags there > as well. I'll prepare a patch. The patch is at [0], thanks. [0] https://patchwork.kernel.org/project/netdevbpf/patch/20231219053150.336991-1-andrii@xxxxxxxxxx/ > > > That's also a perfectly fine traditional unix use (example: socket > > control messages - "struct cmsghdr" with "cmsg_type = SCM_RIGHTS" in > > unix domain sockets). > > > > But if you don't have some other reason for having a separate flag for > > "I also have a file descriptor you should use", then just make a > > negative number mean "no file descriptor". > > > > It's easy to test for the number being negative, but it's also just > > easy to *not* test for, ie it's also perfectly fine to just do > > something like > > > > struct fd f = fdget(fd); > > > > without ever even bothering to test whether 'fd' is negative or not. > > It is guaranteed to fail for negative numbers and just look exactly > > like the "not open" case, so if you don't care about the difference > > between "invalid" and "not open", then a negative fd also works just > > as-is with no extra code at all. > > > > Linus > > > > Linus
