| Message ID | 20240604134347.9357-1-jemmywong512@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improving readability of copy_tree | expand |
On Tue, Jun 04, 2024 at 09:43:47PM +0800, Jemmy wrote: > Hello everyone, > > I'm new to Linux kernel development > and excited to make my first contribution. > While working with the copy_tree function, > I noticed some unclear variable names e.g., p, q, r. > I've updated them to be more descriptive, > aiming to make the code easier to understand. > > Changes: > > p -> o_parent, old parent > q -> n_mnt, new mount > r -> o_mnt, old child > s -> o_child, old child > parent -> n_parent, new parent Hey, seems worth to me but if so please spell out "old_*" and "new_*".
Hello! On Tue 04-06-24 21:43:47, Jemmy wrote: > I'm new to Linux kernel development > and excited to make my first contribution. > While working with the copy_tree function, > I noticed some unclear variable names e.g., p, q, r. > I've updated them to be more descriptive, > aiming to make the code easier to understand. > > Changes: > > p -> o_parent, old parent > q -> n_mnt, new mount > r -> o_mnt, old child > s -> o_child, old child > parent -> n_parent, new parent > > Thanks for the opportunity to be part of this community! So I agree more descriptive names would help readability of this code. But I'd pick different names which IMHO better capture the purpose. mnt -> root (root of the tree to copy) r -> root_child (direct child of the root we are cloning) s -> cur_mnt (current mount we are copying) p -> cur_parent (parent of cur_mnt) q -> cloned_mnt (freshly cloned mount) parent -> cloned_parent Honza > Signed-off-by: Jemmy <jemmywong512@gmail.com> > --- > fs/namespace.c | 51 +++++++++++++++++++++++++------------------------- > 1 file changed, 25 insertions(+), 26 deletions(-) > > diff --git a/fs/namespace.c b/fs/namespace.c > index 5a51315c6678..b1cf95ddfb87 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -1969,7 +1969,7 @@ static bool mnt_ns_loop(struct dentry *dentry) > struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, > int flag) > { > - struct mount *res, *p, *q, *r, *parent; > + struct mount *res, *o_parent, *o_child, *o_mnt, *n_parent, *n_mnt; > > if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt)) > return ERR_PTR(-EINVAL); > @@ -1977,47 +1977,46 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, > if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry)) > return ERR_PTR(-EINVAL); > > - res = q = clone_mnt(mnt, dentry, flag); > - if (IS_ERR(q)) > - return q; > + res = n_mnt = clone_mnt(mnt, dentry, flag); > + if (IS_ERR(n_mnt)) > + return n_mnt; > > - q->mnt_mountpoint = mnt->mnt_mountpoint; > + n_mnt->mnt_mountpoint = mnt->mnt_mountpoint; > > - p = mnt; > - list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) { > - struct mount *s; > - if (!is_subdir(r->mnt_mountpoint, dentry)) > + o_parent = mnt; > + list_for_each_entry(o_mnt, &mnt->mnt_mounts, mnt_child) { > + if (!is_subdir(o_mnt->mnt_mountpoint, dentry)) > continue; > > - for (s = r; s; s = next_mnt(s, r)) { > + for (o_child = o_mnt; o_child; o_child = next_mnt(o_child, o_mnt)) { > if (!(flag & CL_COPY_UNBINDABLE) && > - IS_MNT_UNBINDABLE(s)) { > - if (s->mnt.mnt_flags & MNT_LOCKED) { > + IS_MNT_UNBINDABLE(o_child)) { > + if (o_child->mnt.mnt_flags & MNT_LOCKED) { > /* Both unbindable and locked. */ > - q = ERR_PTR(-EPERM); > + n_mnt = ERR_PTR(-EPERM); > goto out; > } else { > - s = skip_mnt_tree(s); > + o_child = skip_mnt_tree(o_child); > continue; > } > } > if (!(flag & CL_COPY_MNT_NS_FILE) && > - is_mnt_ns_file(s->mnt.mnt_root)) { > - s = skip_mnt_tree(s); > + is_mnt_ns_file(o_child->mnt.mnt_root)) { > + o_child = skip_mnt_tree(o_child); > continue; > } > - while (p != s->mnt_parent) { > - p = p->mnt_parent; > - q = q->mnt_parent; > + while (o_parent != o_child->mnt_parent) { > + o_parent = o_parent->mnt_parent; > + n_mnt = n_mnt->mnt_parent; > } > - p = s; > - parent = q; > - q = clone_mnt(p, p->mnt.mnt_root, flag); > - if (IS_ERR(q)) > + o_parent = o_child; > + n_parent = n_mnt; > + n_mnt = clone_mnt(o_parent, o_parent->mnt.mnt_root, flag); > + if (IS_ERR(n_mnt)) > goto out; > lock_mount_hash(); > - list_add_tail(&q->mnt_list, &res->mnt_list); > - attach_mnt(q, parent, p->mnt_mp, false); > + list_add_tail(&n_mnt->mnt_list, &res->mnt_list); > + attach_mnt(n_mnt, n_parent, o_parent->mnt_mp, false); > unlock_mount_hash(); > } > } > @@ -2028,7 +2027,7 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, > umount_tree(res, UMOUNT_SYNC); > unlock_mount_hash(); > } > - return q; > + return n_mnt; > } > > /* Caller should check returned pointer for errors */ > -- > 2.34.1 >
On 6/4/24 09:43, Jemmy wrote: > Hello everyone, > > I'm new to Linux kernel development > and excited to make my first contribution. > While working with the copy_tree function, > I noticed some unclear variable names e.g., p, q, r. > I've updated them to be more descriptive, > aiming to make the code easier to understand. > > Changes: > > p -> o_parent, old parent > q -> n_mnt, new mount > r -> o_mnt, old child > s -> o_child, old child > parent -> n_parent, new parent > > Thanks for the opportunity to be part of this community! > > BR, > Jemmy I don't know why I am on the to-list as I haven't worked on this file before. Anyway, making meaning name is helpful, but I believe a functional level documentation on top of copy_tree() to explain what this function tries to accomplish can be helpful too. Cheers, Longman > > Signed-off-by: Jemmy <jemmywong512@gmail.com> > --- > fs/namespace.c | 51 +++++++++++++++++++++++++------------------------- > 1 file changed, 25 insertions(+), 26 deletions(-) > > diff --git a/fs/namespace.c b/fs/namespace.c > index 5a51315c6678..b1cf95ddfb87 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -1969,7 +1969,7 @@ static bool mnt_ns_loop(struct dentry *dentry) > struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, > int flag) > { > - struct mount *res, *p, *q, *r, *parent; > + struct mount *res, *o_parent, *o_child, *o_mnt, *n_parent, *n_mnt; > > if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt)) > return ERR_PTR(-EINVAL); > @@ -1977,47 +1977,46 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, > if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry)) > return ERR_PTR(-EINVAL); > > - res = q = clone_mnt(mnt, dentry, flag); > - if (IS_ERR(q)) > - return q; > + res = n_mnt = clone_mnt(mnt, dentry, flag); > + if (IS_ERR(n_mnt)) > + return n_mnt; > > - q->mnt_mountpoint = mnt->mnt_mountpoint; > + n_mnt->mnt_mountpoint = mnt->mnt_mountpoint; > > - p = mnt; > - list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) { > - struct mount *s; > - if (!is_subdir(r->mnt_mountpoint, dentry)) > + o_parent = mnt; > + list_for_each_entry(o_mnt, &mnt->mnt_mounts, mnt_child) { > + if (!is_subdir(o_mnt->mnt_mountpoint, dentry)) > continue; > > - for (s = r; s; s = next_mnt(s, r)) { > + for (o_child = o_mnt; o_child; o_child = next_mnt(o_child, o_mnt)) { > if (!(flag & CL_COPY_UNBINDABLE) && > - IS_MNT_UNBINDABLE(s)) { > - if (s->mnt.mnt_flags & MNT_LOCKED) { > + IS_MNT_UNBINDABLE(o_child)) { > + if (o_child->mnt.mnt_flags & MNT_LOCKED) { > /* Both unbindable and locked. */ > - q = ERR_PTR(-EPERM); > + n_mnt = ERR_PTR(-EPERM); > goto out; > } else { > - s = skip_mnt_tree(s); > + o_child = skip_mnt_tree(o_child); > continue; > } > } > if (!(flag & CL_COPY_MNT_NS_FILE) && > - is_mnt_ns_file(s->mnt.mnt_root)) { > - s = skip_mnt_tree(s); > + is_mnt_ns_file(o_child->mnt.mnt_root)) { > + o_child = skip_mnt_tree(o_child); > continue; > } > - while (p != s->mnt_parent) { > - p = p->mnt_parent; > - q = q->mnt_parent; > + while (o_parent != o_child->mnt_parent) { > + o_parent = o_parent->mnt_parent; > + n_mnt = n_mnt->mnt_parent; > } > - p = s; > - parent = q; > - q = clone_mnt(p, p->mnt.mnt_root, flag); > - if (IS_ERR(q)) > + o_parent = o_child; > + n_parent = n_mnt; > + n_mnt = clone_mnt(o_parent, o_parent->mnt.mnt_root, flag); > + if (IS_ERR(n_mnt)) > goto out; > lock_mount_hash(); > - list_add_tail(&q->mnt_list, &res->mnt_list); > - attach_mnt(q, parent, p->mnt_mp, false); > + list_add_tail(&n_mnt->mnt_list, &res->mnt_list); > + attach_mnt(n_mnt, n_parent, o_parent->mnt_mp, false); > unlock_mount_hash(); > } > } > @@ -2028,7 +2027,7 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, > umount_tree(res, UMOUNT_SYNC); > unlock_mount_hash(); > } > - return q; > + return n_mnt; > } > > /* Caller should check returned pointer for errors */
diff --git a/fs/namespace.c b/fs/namespace.c index 5a51315c6678..b1cf95ddfb87 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -1969,7 +1969,7 @@ static bool mnt_ns_loop(struct dentry *dentry) struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, int flag) { - struct mount *res, *p, *q, *r, *parent; + struct mount *res, *o_parent, *o_child, *o_mnt, *n_parent, *n_mnt; if (!(flag & CL_COPY_UNBINDABLE) && IS_MNT_UNBINDABLE(mnt)) return ERR_PTR(-EINVAL); @@ -1977,47 +1977,46 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, if (!(flag & CL_COPY_MNT_NS_FILE) && is_mnt_ns_file(dentry)) return ERR_PTR(-EINVAL); - res = q = clone_mnt(mnt, dentry, flag); - if (IS_ERR(q)) - return q; + res = n_mnt = clone_mnt(mnt, dentry, flag); + if (IS_ERR(n_mnt)) + return n_mnt; - q->mnt_mountpoint = mnt->mnt_mountpoint; + n_mnt->mnt_mountpoint = mnt->mnt_mountpoint; - p = mnt; - list_for_each_entry(r, &mnt->mnt_mounts, mnt_child) { - struct mount *s; - if (!is_subdir(r->mnt_mountpoint, dentry)) + o_parent = mnt; + list_for_each_entry(o_mnt, &mnt->mnt_mounts, mnt_child) { + if (!is_subdir(o_mnt->mnt_mountpoint, dentry)) continue; - for (s = r; s; s = next_mnt(s, r)) { + for (o_child = o_mnt; o_child; o_child = next_mnt(o_child, o_mnt)) { if (!(flag & CL_COPY_UNBINDABLE) && - IS_MNT_UNBINDABLE(s)) { - if (s->mnt.mnt_flags & MNT_LOCKED) { + IS_MNT_UNBINDABLE(o_child)) { + if (o_child->mnt.mnt_flags & MNT_LOCKED) { /* Both unbindable and locked. */ - q = ERR_PTR(-EPERM); + n_mnt = ERR_PTR(-EPERM); goto out; } else { - s = skip_mnt_tree(s); + o_child = skip_mnt_tree(o_child); continue; } } if (!(flag & CL_COPY_MNT_NS_FILE) && - is_mnt_ns_file(s->mnt.mnt_root)) { - s = skip_mnt_tree(s); + is_mnt_ns_file(o_child->mnt.mnt_root)) { + o_child = skip_mnt_tree(o_child); continue; } - while (p != s->mnt_parent) { - p = p->mnt_parent; - q = q->mnt_parent; + while (o_parent != o_child->mnt_parent) { + o_parent = o_parent->mnt_parent; + n_mnt = n_mnt->mnt_parent; } - p = s; - parent = q; - q = clone_mnt(p, p->mnt.mnt_root, flag); - if (IS_ERR(q)) + o_parent = o_child; + n_parent = n_mnt; + n_mnt = clone_mnt(o_parent, o_parent->mnt.mnt_root, flag); + if (IS_ERR(n_mnt)) goto out; lock_mount_hash(); - list_add_tail(&q->mnt_list, &res->mnt_list); - attach_mnt(q, parent, p->mnt_mp, false); + list_add_tail(&n_mnt->mnt_list, &res->mnt_list); + attach_mnt(n_mnt, n_parent, o_parent->mnt_mp, false); unlock_mount_hash(); } } @@ -2028,7 +2027,7 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry, umount_tree(res, UMOUNT_SYNC); unlock_mount_hash(); } - return q; + return n_mnt; } /* Caller should check returned pointer for errors */
Hello everyone, I'm new to Linux kernel development and excited to make my first contribution. While working with the copy_tree function, I noticed some unclear variable names e.g., p, q, r. I've updated them to be more descriptive, aiming to make the code easier to understand. Changes: p -> o_parent, old parent q -> n_mnt, new mount r -> o_mnt, old child s -> o_child, old child parent -> n_parent, new parent Thanks for the opportunity to be part of this community! BR, Jemmy Signed-off-by: Jemmy <jemmywong512@gmail.com> --- fs/namespace.c | 51 +++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 26 deletions(-)