| Message ID | 20230302182207.456311-2-aloktiagi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/3] file: Introduce iterate_fd_locked | expand |
From: aloktiagi > Sent: 02 March 2023 18:22 > > Allow callers of do_dup2 to free the old file descriptor in case they need to > make additional operations on it. That doesn't read right at all. Whether or not this is a good idea (or can be done differently) the interface is horrid. > if (tofree) > - filp_close(tofree, files); > + *fdfile = tofree; Why not: if (fdfile) [ *fdfile = tofree; } else { if (tofree) filp_close(tofree, files); } Then existing code just passes NULL and the caller can't 'forget' to intitalise fdfile. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Mar 02, 2023 at 10:21:11PM +0000, David Laight wrote: > From: aloktiagi > > Sent: 02 March 2023 18:22 > > > > Allow callers of do_dup2 to free the old file descriptor in case they need to > > make additional operations on it. > > That doesn't read right at all. > > Whether or not this is a good idea (or can be done differently) > the interface is horrid. > > > if (tofree) > > - filp_close(tofree, files); > > + *fdfile = tofree; > > Why not: > > if (fdfile) [ > *fdfile = tofree; > } else { > if (tofree) > filp_close(tofree, files); > } > > Then existing code just passes NULL and the caller can't 'forget' > to intitalise fdfile. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > thank you for the review, I'll fix it in v2
diff --git a/fs/file.c b/fs/file.c index 4b2346b8a5ee..04c8775d337a 100644 --- a/fs/file.c +++ b/fs/file.c @@ -1086,7 +1086,7 @@ bool get_close_on_exec(unsigned int fd) } static int do_dup2(struct files_struct *files, - struct file *file, unsigned fd, unsigned flags) + struct file *file, unsigned fd, struct file **fdfile, unsigned flags) __releases(&files->file_lock) { struct file *tofree; @@ -1120,7 +1120,7 @@ __releases(&files->file_lock) spin_unlock(&files->file_lock); if (tofree) - filp_close(tofree, files); + *fdfile = tofree; return fd; @@ -1132,6 +1132,7 @@ __releases(&files->file_lock) int replace_fd(unsigned fd, struct file *file, unsigned flags) { int err; + struct file *fdfile = NULL; struct files_struct *files = current->files; if (!file) @@ -1144,7 +1145,10 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags) err = expand_files(files, fd); if (unlikely(err < 0)) goto out_unlock; - return do_dup2(files, file, fd, flags); + err = do_dup2(files, file, fd, &fdfile, flags); + if (fdfile) + filp_close(fdfile, files); + return err; out_unlock: spin_unlock(&files->file_lock); @@ -1216,6 +1220,7 @@ static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags) { int err = -EBADF; struct file *file; + struct file *fdfile = NULL; struct files_struct *files = current->files; if ((flags & ~O_CLOEXEC) != 0) @@ -1237,7 +1242,10 @@ static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags) goto Ebadf; goto out_unlock; } - return do_dup2(files, file, newfd, flags); + err = do_dup2(files, file, newfd, &fdfile, flags); + if (fdfile) + filp_close(fdfile, files); + return err; Ebadf: err = -EBADF;
Allow callers of do_dup2 to free the old file descriptor in case they need to make additional operations on it. Signed-off-by: aloktiagi <aloktiagi@gmail.com> --- fs/file.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)