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

Fix zfs-mount-generator escaping #9970

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

rlaager
Copy link
Member

@rlaager rlaager commented Feb 8, 2020

Motivation and Context

This fixes the name of the mount unit for the root filesystem. I discovered these pre-existing issues in review/testing of #9649.

In practice, the root filesystem is mounted by the initrd anyway, so this is moot in practice (which is why this wasn't noticed), but we should be correct nonetheless.

Description

The correct name for the mount unit for / is "-.mount", not ".mount".

How Has This Been Tested?

I used some print statements and some diffing. This is a nice test case, if it works on your particular system:

rm -rf /tmp/before
mkdir /tmp/before
sudo /lib/systemd/system-generators/zfs-mount-generator /tmp/before /tmp/before /tmp/before

rm -rf /tmp/after
mkdir /tmp/after
sudo /lib/systemd/system-generators/zfs-mount-generator /tmp/after /tmp/after /tmp/after

diff -urN /tmp/before /tmp/after

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@rlaager rlaager changed the title Generator escaping Fix zfs-mount-generator escaping Feb 8, 2020
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 10, 2020
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

This looks correct, but I did not manually test it.

@behlendorf
Copy link
Contributor

@aerusso would you mind taking a look at this change.

@rlaager rlaager force-pushed the generator-escaping branch 2 times, most recently from 535a51f to 9445e30 Compare February 12, 2020 20:52
@rlaager
Copy link
Member Author

rlaager commented Feb 12, 2020

I have changed to use the more proper form of escaping, as suggested by @aerusso. This also eliminates a lot of the need for the first commit. I'm happy with this simpler change.

@rlaager rlaager requested a review from behlendorf February 12, 2020 20:53
The correct name for the mount unit for / is "-.mount", not ".mount".

Co-authored-by: Antonio Russo <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
@rlaager
Copy link
Member Author

rlaager commented Feb 12, 2020

@aerusso Since you basically "rewrote" this one-liner, I gave you a Co-authored-by credit here. I can make you the sole author if you'd prefer.

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #9970 into master will increase coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #9970    +/-   ##
========================================
+ Coverage      79%      79%   +<1%     
========================================
  Files         385      385            
  Lines      122382   122382            
========================================
+ Hits        96922    97092   +170     
+ Misses      25460    25290   -170
Flag Coverage Δ
#kernel 79% <ø> (ø) ⬆️
#user 67% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12f7b90...ba453cd. Read the comment docs.

Copy link
Contributor

@aerusso aerusso left a comment

Choose a reason for hiding this comment

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

This looks good! The co-authorship is fine, too.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 13, 2020
@behlendorf behlendorf merged commit d1d65bb into openzfs:master Feb 13, 2020
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Apr 22, 2020
The correct name for the mount unit for / is "-.mount", not ".mount".

Reviewed-by: InsanePrawn <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Co-authored-by: Antonio Russo <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes openzfs#9970
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Apr 22, 2020
The correct name for the mount unit for / is "-.mount", not ".mount".

Reviewed-by: InsanePrawn <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Co-authored-by: Antonio Russo <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes openzfs#9970
tonyhutter pushed a commit that referenced this pull request May 12, 2020
The correct name for the mount unit for / is "-.mount", not ".mount".

Reviewed-by: InsanePrawn <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Co-authored-by: Antonio Russo <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes #9970
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
The correct name for the mount unit for / is "-.mount", not ".mount".

Reviewed-by: InsanePrawn <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Co-authored-by: Antonio Russo <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes openzfs#9970
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants