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

Improve Uid & Gid support #40

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

Conversation

manvalls
Copy link

@manvalls manvalls commented Mar 3, 2018

The purpose of this PR is to allow file systems to:

  1. Create inodes with the UID and GID of the calling process
  2. Support chown

Copy link
Owner

@jacobsa jacobsa 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 doing this. Please add tests to the appropriate cases in samples/memfs/memfs_test.go, and probably add a new case for Chown.

@manvalls manvalls force-pushed the feature/better-uid-gid branch 3 times, most recently from b4c41b3 to c8197cf Compare March 5, 2018 21:00
@manvalls
Copy link
Author

manvalls commented Mar 5, 2018

Test case for chown turned to be a pain in the ass, as you need to be root to actually use it. You can see my latest attempt here, it worked locally on my desktop but travis wasn't happy about it and neither was osx.

Also some test cases seem to hang on linux, no clue about those, seems to be unrelated to the change. Anyway, that's as far as I go, feel free to pick it up from here.

@stapelberg
Copy link
Collaborator

Would you mind rebasing this on the current master branch please? We fixed the travis config in the meantime.

@manvalls manvalls force-pushed the feature/better-uid-gid branch from c8197cf to 58a7930 Compare September 21, 2019 10:29
The purpose of this PR is to allow file systems to:
1. Create inodes with the UID and GID of the calling process
2. Support chown
@manvalls manvalls force-pushed the feature/better-uid-gid branch from 58a7930 to 13353bc Compare September 21, 2019 10:32
switch inMsg.Header().Opcode {
header := inMsg.Header()

switch header.Opcode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: use switch header := inMsg.Header(); header.Opcode to scope the header variable to the switch block only (smaller scopes are generally preferable)

@@ -150,6 +150,8 @@ type SetInodeAttributesOp struct {
Mode *os.FileMode
Atime *time.Time
Mtime *time.Time
Uid *uint32
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why you’re adding Uid/Gid only to some operations, but not to all? Isn’t the uid/gid field set by the kernel for all requests?

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