Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helpers.linux.kernfs: add kernfs_children() and follow_symlinks support #449

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kylee0215
Copy link
Contributor

Add kernfs_children() to iterate over the children of a directory in kernfs.
Add support of follow_symlinks.

Add kernfs_children() to iterate all kernfs_node
in children rb_tree.

Signed-off-by: Kuan-Ying Lee <kuan-ying.lee@canonical.com>
Extend kernfs_walk() to support symlink.

Signed-off-by: Kuan-Ying Lee <kuan-ying.lee@canonical.com>
Copy link
Contributor

@brenns10 brenns10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this all looks good, just one question regarding why to only follow symlinks at the end of the path.

Comment on lines +72 to +74
:param follow_symlinks: If follow_symlinks is ``False``, and the
last component of a path is a symlink, the function will
return ``struct kernfs_node *`` of the symbolic link.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only the last component?

I guess I don't know well enough whether there are cases where symlinks would be in the middle of a real kernfs path. So maybe it won't make a difference. It just seems reasonable to support it within the path too.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That probably came from my description in #443. My intention was that internal symlinks are always followed, and the last one is controlled by the parameter, just like O_NOFOLLOW and the os module. I haven't read the code to see if there was a misunderstanding.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, back from vacation and read the code now. Yes, this needs to be changed to always follow symlinks that are not the last component.

Copy link
Owner

@osandov osandov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! A couple of comments.

Comment on lines +103 to +106
for child in rbtree_inorder_for_each_entry(
"struct kernfs_node", kn.dir.children.address_of_(), "rb"
):
yield child
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor simplification:

Suggested change
for child in rbtree_inorder_for_each_entry(
"struct kernfs_node", kn.dir.children.address_of_(), "rb"
):
yield child
return rbtree_inorder_for_each_entry(
"struct kernfs_node", kn.dir.children.address_of_(), "rb"
)

Comment on lines +72 to +74
:param follow_symlinks: If follow_symlinks is ``False``, and the
last component of a path is a symlink, the function will
return ``struct kernfs_node *`` of the symbolic link.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, back from vacation and read the code now. Yes, this needs to be changed to always follow symlinks that are not the last component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants