| Message ID | tencent_63C013752AD7CA1A22E75CEF6166442E6D05@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fs: modify the annotation of vfs_mkdir() in fs/namei.c | expand |
On Sat, Jun 15, 2024 at 12:38:32PM +0800, Congjie Zhou wrote:
> modify the annotation of @dir and @dentry
Well, it is clearly obvious that you modify them from the patch. But
why?
The @dentry is actually the dentry of child directory, but the origin annotation indicates it is dentry of parent directory. The @dir is similar.
On Fri, Jun 14, 2024 at 11:29:38PM -0700, Christoph Hellwig wrote: > On Sat, Jun 15, 2024 at 12:38:32PM +0800, Congjie Zhou wrote: > > modify the annotation of @dir and @dentry > > Well, it is clearly obvious that you modify them from the patch. But > why? Look at the current comment: * @dir: inode of @dentry It is an inode of _some_ dentry; it's most definitely not that of the argument named 'dentry'. * @dentry: pointer to dentry of the base directory No. The first thing vfs_mkdir() does is int error = may_create(idmap, dir, dentry); if (error) return error; Look at may_create(). Starts with static inline int may_create(struct mnt_idmap *idmap, struct inode *dir, struct dentry *child) { audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE); if (child->d_inode) return -EEXIST; If the last argument (aka. 'dentry' argument of vfs_mkdir()) is currently referring to *ANY* directory, you get -EEXIST. For a good and simple reason: it is the dentry of subdirectory to be created. Now, the second argument of vfs_mkdir() ('dir') is the inode of the parent to be (or base directory, if you will). While we are at it, the rest of comments coming from the same commit suffer similar problems. vfs_create(), vfs_symlink(), et.al. vfs_unlink() is fine, vfs_rmdir() should match vfs_unlink() (inode of parent + dentry of victim).
On Sat, Jun 15, 2024 at 07:55:28AM +0100, Al Viro wrote: > It is an inode of _some_ dentry; it's most definitely not that > of the argument named 'dentry'. No need to explain it here, the point was that this belongs into a useful commit message.
On Sat, Jun 15, 2024 at 12:31:49AM -0700, Christoph Hellwig wrote: > On Sat, Jun 15, 2024 at 07:55:28AM +0100, Al Viro wrote: > > It is an inode of _some_ dentry; it's most definitely not that > > of the argument named 'dentry'. > > No need to explain it here, the point was that this belongs into a > useful commit message. Seeing that fsdevel is archived, it might be worth spelling out, actually... Anyway, yes, something like "correct the inline descriptions of vfs_mkdir() et.al." ought to go into commit message. And it really ought to cover not just vfs_mkdir() - other similar comments from the same commit (6521f8917082 "namei: prepare for idmapped mounts") have similar issues. "Create a device node or file" for vfs_mknod() probably ought to be "Create a device node or other special file", while we are at it...
diff --git a/fs/namei.c b/fs/namei.c index 37fb0a8aa..2fd3ba6a4 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -4095,8 +4095,8 @@ SYSCALL_DEFINE3(mknod, const char __user *, filename, umode_t, mode, unsigned, d /** * vfs_mkdir - create directory * @idmap: idmap of the mount the inode was found from - * @dir: inode of @dentry - * @dentry: pointer to dentry of the base directory + * @dir: inode of the parent directory + * @dentry: dentry of the child directory * @mode: mode of the new directory * * Create a directory.
modify the annotation of @dir and @dentry Signed-off-by: Congjie Zhou <zcjie0802@qq.com> --- fs/namei.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)