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

passthrough example for ext fs #96

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

Conversation

kuchling
Copy link

@kuchling kuchling commented Nov 23, 2023

Ext filesystem compatibility checked by https://github.com/zfsonlinux/fstest.git. I don't know how good this testsuit is, but i also found no better or new one.
Could be merged, i have only tested with ext4 as source fs. This testsuit has a lot of testcases.

  • create syscal didn't set mode and ownership
  • chown syscal with no change of uid gid should mofify ctime on ext fs (seems to be different on other filesystems)
  • allow_other, dev, suid was set within options to run all testcases

@Nikratio
Copy link
Contributor

Thanks! Could you elaborate on the differences to the existing passthroughfs.py? Might there be a way to merge the changes in there instead?

@kuchling
Copy link
Author

Thanks! Could you elaborate on the differences to the existing passthroughfs.py? Might there be a way to merge the changes in there instead?

Ext filesystem compatibility checked by https://github.com/zfsonlinux/fstest.git. I don't know how good this testsuit is, but i also found no better or new one.
Could be merged, i have only tested with ext4 as source fs. This testsuit has a lot of testcases.

create syscal didn't set mode and ownership
chown syscal with no change of uid gid should mofify ctime on ext fs (seems to be different on other filesystems)
allow_other, dev, suid was set within options to run all testcases

@kuchling kuchling closed this Nov 27, 2023
@kuchling kuchling reopened this Nov 27, 2023
@Nikratio
Copy link
Contributor

Thanks! Could you elaborate on the differences to the existing passthroughfs.py? Might there be a way to merge the changes in there instead?

Ext filesystem compatibility checked by https://github.com/zfsonlinux/fstest.git.

To rephrase, the difference between the existing passthroughfs.py and your ext_passthroughfs.py is that the latter passes the fstest, and the former does not?

If so, what would it take to modify passthroughfs.py to pass the test? Why do we need a new filesystem?

@kuchling
Copy link
Author

I do not need a new fs, it was choosen to not break other stuff and because different lower fs have different behavoir see second change, regarding time. The only relevant change for passthroughps.py in general in my opinion is to correct the create syscall as stated to fix owner and mode.
Why the testsuite? It was to get maximum compatiblility with the original fs, means if passthroughfs.py will be used to intercept some things (every ones one fun and idea) it should behave as good as possible to the original fs.

@Nikratio
Copy link
Contributor

I do not need a new fs, it was choosen to not break other stuff and because different lower fs have different behavoir see second change, regarding time.

I do not see any changes, I just see one large new file being added.

Could you please modify passthroughfs.py instead of adding a new file?

Copy link
Collaborator

@ThomasWaldmann ThomasWaldmann 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 the updated PR, some feedback from me.

Comment on lines +316 to +322
if fields.update_mode is False and \
fields.update_uid is False and \
fields.update_gid is False:
if chown == os.chown:
chown(path_or_fh, -1, -1, follow_symlinks=False)
if chown == os.fchown:
chown(path_or_fh, -1, -1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, this is rather strange:

  • why is chown called at all if neither uid nor gid shall be set (both are -1)?
  • is the x is False really wanted / correct instead of the usual boolean evaluation using not x? consider x == 0 or x == None, for example.
  • in 311/314 chown is always called with follow_symlinks=False, no matter whether path or fh is used, so 320/322 look inconsistent with these.

@@ -479,3 +493,4 @@ def main():

if __name__ == '__main__':
main()

Copy link
Collaborator

Choose a reason for hiding this comment

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

accidental change?

@@ -1,5 +1,6 @@
#!/usr/bin/env python3
'''
ext filesystem compatibility checked by https://github.com/zfsonlinux/fstest.git
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is rather a remark or property of this implementation, but nothing the description of this should start with, so move it a bit lower, please.

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