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

Implement Trenameat and Tunlinkat #135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JJL772
Copy link

@JJL772 JJL772 commented Feb 17, 2025

Implements Tunlinkat and Trenameat, ironically not using either of those syscalls.

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

Couple of things:

  • could you update the src/libnpfs/test/encoding.c to make sure we're checking serialize/deserialize for these operations
  • would you mind expanding the commit message to provide any back story or references that might be useful for context? (Why were you motivated to add this, what problem does this solve for you, etc)
  • The test framework in tests needs an overhaul (and I have a partial branch doing that on the back burner) so I don't want to ask you to add another test, but perhaps if you have an idea of how we would test this with the kernel 9p client we could open an issue to add a test later with your notes.

I'll try to do a proper review in the next couple of days, including refreshing my memory on how those syscalls work and what's going on in the linux kernel 9p side. Any pointers/context you can share in the mean time would be appreciated.

@garlick
Copy link
Member

garlick commented Feb 18, 2025

could you update the src/libnpfs/test/encoding.c to make sure we're checking serialize/deserialize for these operations

Duh! Never mind, already there.

@JJL772
Copy link
Author

JJL772 commented Feb 19, 2025

Thanks for the initial feedback!

would you mind expanding the commit message to provide any back story or references that might be useful for context? (Why were you motivated to add this, what problem does this solve for you, etc)

My motivation was that I implemented Trenameat/Tunlinkat in a 9p client before realizing that diod returns ENOTSUPP for both. Since the boilerplate was already there for these calls, I figured that I'd try to implement them. I had chosen Trenameat/Tunlinkat because they were well suited for implementing a couple file system operation handlers on RTEMS, which is ultimately where this 9p client will be deployed.

The test framework in tests needs an overhaul (and I have a partial branch doing that on the back burner) so I don't want to ask you to add another test, but perhaps if you have an idea of how we would test this with the kernel 9p client we could open an issue to add a test later with your notes.

Kernel 9p client function v9fs_vfs_remove looks like it tries Tunlinkat first, and falls back to Tremove if not supported. The existing tests might cover this already in that case, at the cost of overlapping with Tremove. Same situation with v9fs_vfs_rename; it first tries Trenameat and uses Trename if it returns -EOPNOTSPP

I'll play around with the tests and update the commit message a bit later this week.

@JJL772
Copy link
Author

JJL772 commented Feb 20, 2025

After running sudo make check, tests/kern/t31.diod shows a bunch of Trenameat and Tunlinkat calls.

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.

2 participants