On Thu, Apr 27, 2023 at 08:21:34AM -0700, Linus Torvalds wrote: > On Thu, Apr 27, 2023 at 12:39 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > > > int delayed_dup(struct file *file, unsigned flags) > > Ok, this is strange. Let me think about it. > > But even without thinking about it, this part I hate: > > > struct delayed_dup *p = kmalloc(sizeof(struct delayed_dup), GFP_KERNEL); > > Sure, if this is only used in unimportant code where performance > doesn't matter, doing a kmalloc is fine. > > But if that is the only use, I think this is too subtle an interface. Still hadn't finished with the zoo... > Could we instead limit it to "we only have one pending delayed dup", > and make this all be more like the restart-block thing, and be part of > struct task_struct? Interesting... FWIW, *anything* that wants several descriptors has special needs - there are some places like that (binder, for one) and they have rather weird code implementing those. Just to restate the obvious: this is not applicable for the most frequent caller - open(2). For the reasons that have nothing to do with performance. If opening the file has hard-to-reverse side effects (like directory modification due to O_CREAT), the things are very different. What I hope for is a small number of patterns, with clear rules for choosing the one that is applicable and helpers for each that would reduce the amount of headache when using it. And I've no problem with "this particular pattern is not usable if you are adding more than one descriptor" - that's not hard to understand and verify. So I'm fine with doing that for one descriptor only and getting rid of the allocation. BTW, another pattern is the same sans the "delayed" part. I.e. "here's an opened file, get a descriptor and either attach the file to it or fput() the damn thing; in any case, file reference is consumed and descriptor-or-error is returned". That one is definitely only for single descriptor case. In any case, I want to finish the survey of the callers first, just to see what's there and whether anything is worth nicking. While we are at it, I want to make close_fd() use a very big red flag. To the point of grepping for new callers in -next and asking the folks who introduce those to explain WTF they are doing...
