Fix bug in in_mtab() where the return value is always 1 #9168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
$fs used with the wrong sed command where should be $mntpnt instead to match a variable exported by read_mtab() The fix is mostly to reuse the sed command found in read_mtab() Signed-off-by: Alexey Smirnoff <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #9168 +/- ##
==========================================
+ Coverage 79.09% 79.13% +0.04%
==========================================
Files 400 400
Lines 121831 121831
==========================================
+ Hits 96361 96410 +49
+ Misses 25470 25421 -49
Continue to review full report at Codecov.
|
behlendorf
approved these changes
Aug 16, 2019
|
@c0d3z3r0 would you mind reviewing this. |
c0d3z3r0
suggested changes
Aug 17, 2019
There was a problem hiding this comment.
This fix is valid but I guess we should go another way here. While testing and verifying this PR I noticed that most the implementation of zfs-functions et al. generally has multiple problems. Main reasons for that are:
- hardly readable because of too much unoptimized code
- inconsistent coding style, e.g.
[ "$(eval echo "$""$var")" != "" ]vs.var="$(eval echo FSTAB_$1)"; [ "${var}" != "" ] - use of
evalwhere it is not needed at all, e.g.eval export FSTAB_dev_$i="$fs" - duplicated functionality:
in_mtabvs.is_mounted - inconsistent use of
mountandcat/read /proc/self/mounts(which both return the same results. the only differene is thatmountdoes some formatting (insertonandtype, put options in brackets, omitting freq and pass)
...
I vote for a complete reimplementation of zfs-functions by using (e)grep /proc/self/mounts or even zfs_mount (have to check this) and a proper, consistent coding style avoiding any use of eval, where possible. I will post a new PR for that in the next few days I guess
c0d3z3r0
added a commit
to c0d3z3r0/zfs
that referenced
this pull request
Aug 19, 2019
This fixes multiple issues with shell scripts: - current shell code is hardly readable because of too much unoptimized code - some bugs (like openzfs#9168) - inconsistent coding style - very much missing documentation - use of eva... evil() where it is not needed at all, e.g.: `eval export FSTAB_dev_$i="$fs"` - duplicated functionality: in_mtab vs. is_mounted - inconsistent use of mount and cat/read /proc/self/mounts (which both return the same results. the only differene is that mount does some formatting (insert on and type, put options in brackets, omitting freq and pass) - caching of fstab and mtab in env makes everything very, very complicated and is not needed at all (as far as I looked at the code; MAYBE this is really needed and I did not get this far, yet. we'll see) This PR does a complete rework of all existing shell scripts to fix all those problems. Signed-off-by: Michael Niewöhner <[email protected]>
12 tasks
c0d3z3r0
added a commit
to c0d3z3r0/zfs
that referenced
this pull request
Aug 19, 2019
This fixes multiple issues with shell scripts: - current shell code is hardly readable because of too much unoptimized code - some bugs (like openzfs#9168) - inconsistent coding style - very much missing documentation - use of eva... evil() where it is not needed at all, e.g.: `eval export FSTAB_dev_$i="$fs"` - duplicated functionality: in_mtab vs. is_mounted - inconsistent use of mount and cat/read /proc/self/mounts (which both return the same results. the only differene is that mount does some formatting (insert on and type, put options in brackets, omitting freq and pass) - caching of fstab and mtab in env makes everything very, very complicated and is not needed at all; (as far as I looked at the code; MAYBE this is really needed and I did not get this far, yet. we'll see) IMHO it really does not matter if we call grep multiple times... the difference is negligible and should be in the range of some 10s or 100s of milliseconds... This PR does a complete rework of all existing shell scripts to fix all those problems while keeping backwards compatibility. Signed-off-by: Michael Niewöhner <[email protected]>
c0d3z3r0
added a commit
to c0d3z3r0/zfs
that referenced
this pull request
Aug 19, 2019
This fixes multiple issues with shell scripts: - current shell code is hardly readable because of too much unoptimized code - some bugs (like openzfs#9168, openzfs#9183) - inconsistent coding style - very much missing documentation - use of eva... evil() where it is not needed at all, e.g.: `eval export FSTAB_dev_$i="$fs"` - duplicated functionality: in_mtab vs. is_mounted - inconsistent use of mount and cat/read /proc/self/mounts (which both return the same results. the only differene is that mount does some formatting (insert on and type, put options in brackets, omitting freq and pass) - caching of fstab and mtab in env makes everything very, very complicated and is not needed at all; (as far as I looked at the code; MAYBE this is really needed and I did not get this far, yet. we'll see) IMHO it really does not matter if we call grep multiple times... the difference is negligible and should be in the range of some 10s or 100s of milliseconds... This PR does a complete rework of all existing shell scripts to fix all those problems while keeping backwards compatibility. Signed-off-by: Michael Niewöhner <[email protected]>
c0d3z3r0
approved these changes
Aug 19, 2019
c0d3z3r0
added a commit
to c0d3z3r0/zfs
that referenced
this pull request
Aug 19, 2019
This fixes multiple issues with shell scripts: - current shell code is hardly readable because of too much unoptimized code - some bugs (like openzfs#9168, openzfs#9183) - inconsistent coding style - use of UPPER CASE variables (very confusing as only env vars or globals should be upper case... - very much missing documentation - use of eva... evil() where it is not needed at all, e.g.: `eval export FSTAB_dev_$i="$fs"` - duplicated functionality: in_mtab vs. is_mounted - inconsistent use of mount and cat/read /proc/self/mounts (which both return the same results. the only differene is that mount does some formatting (insert on and type, put options in brackets, omitting freq and pass) - caching of fstab and mtab in env makes everything very, very complicated and is not needed at all; (as far as I looked at the code; MAYBE this is really needed and I did not get this far, yet. we'll see) IMHO it really does not matter if we call grep multiple times... the difference is negligible and should be in the range of some 10s or 100s of milliseconds... This PR does a complete rework of all existing shell scripts to fix all those problems while keeping backwards compatibility. Signed-off-by: Michael Niewöhner <[email protected]>
c0d3z3r0
added a commit
to c0d3z3r0/zfs
that referenced
this pull request
Aug 20, 2019
The shell scripts zfs-functions et al. have multiple problems: - Current shell code is hard to read and debug because of too much unoptimized code - Some bugs (like openzfs#9168, openzfs#9183, and I guess some more but I did not search in the issues, yet) - Inconsistent code style - Use of upper case variables (very confusing at least for me, as only env vars or globals should be upper case (isn't this (un)written convention?) - Very much missing documentation which makes it hard for new devs to understand what SHOULD happen (which is not always what really happens) - use of evi... eval() where it is not needed at all, e.g.: `eval export FSTAB_dev_$i="$fs"` - duplicated functionality: `in_mtab` vs. `is_mounted` - inconsistent use of `mount` and `cat/read /proc/self/mounts` (which both return the same results. The only differene is that mount does some formatting (insert `on` and `type`, put `options` in brackets, omitting `freq` and `pass`) - Caching of fstab and mtab in env makes everything very, very complicated due to special character handling and (as far as I looked at the code, yet) is not needed at all. (MAYBE this is really needed and I did not get this far, yet. We'll see.) IMHO it really does not matter if we call grep multiple times but can completely drop any `eval`. The run time difference is negligible and should be in the range of some 10s or 100s of milliseconds. - ... (more to follow) This PR does a complete rework of all existing shell scripts to fix all those problems while keeping backwards compatibility. Signed-off-by: Michael Niewöhner <[email protected]>
behlendorf
added
Status: Accepted
c0d3z3r0
added a commit
to c0d3z3r0/zfs
that referenced
this pull request
Aug 21, 2019
The shell scripts zfs-functions et al. have multiple problems: - Current shell code is hard to read and debug because of too much unoptimized code - Some bugs (like openzfs#9168, openzfs#9183, and I guess some more but I did not search in the issues, yet) - Inconsistent code style - Use of upper case variables (very confusing at least for me, as only env vars or globals should be upper case (isn't this (un)written convention?) - Very much missing documentation which makes it hard for new devs to understand what SHOULD happen (which is not always what really happens) - use of evi... eval() where it is not needed at all, e.g.: `eval export FSTAB_dev_$i="$fs"` - duplicated functionality: `in_mtab` vs. `is_mounted` - inconsistent use of `mount` and `cat/read /proc/self/mounts` (which both return the same results. The only differene is that mount does some formatting (insert `on` and `type`, put `options` in brackets, omitting `freq` and `pass`) - Caching of fstab and mtab in env makes everything very, very complicated due to special character handling and (as far as I looked at the code, yet) is not needed at all. (MAYBE this is really needed and I did not get this far, yet. We'll see.) IMHO it really does not matter if we call grep multiple times but can completely drop any `eval`. The run time difference is negligible and should be in the range of some 10s or 100s of milliseconds. - ... (more to follow) This PR does a complete rework of all existing shell scripts to fix all those problems while keeping backwards compatibility. Signed-off-by: Michael Niewöhner <[email protected]>
c0d3z3r0
added a commit
to c0d3z3r0/zfs
that referenced
this pull request
Aug 21, 2019
The shell scripts zfs-functions et al. have multiple problems: - Current shell code is hard to read and debug because of too much unoptimized code - Some bugs (like openzfs#9168, openzfs#9183, and I guess some more but I did not search in the issues, yet) - Inconsistent code style - Use of upper case variables (very confusing at least for me, as only env vars or globals should be upper case (isn't this (un)written convention?) - Very much missing documentation which makes it hard for new devs to understand what SHOULD happen (which is not always what really happens) - use of evi... eval() where it is not needed at all, e.g.: `eval export FSTAB_dev_$i="$fs"` - duplicated functionality: `in_mtab` vs. `is_mounted` - inconsistent use of `mount` and `cat/read /proc/self/mounts` (which both return the same results. The only differene is that mount does some formatting (insert `on` and `type`, put `options` in brackets, omitting `freq` and `pass`) - Caching of fstab and mtab in env makes everything very, very complicated due to special character handling and (as far as I looked at the code, yet) is not needed at all. (MAYBE this is really needed and I did not get this far, yet. We'll see.) IMHO it really does not matter if we call grep multiple times but can completely drop any `eval`. The run time difference is negligible and should be in the range of some 10s or 100s of milliseconds. - ... (more to follow) This PR does a complete rework of all existing shell scripts to fix all those problems while keeping backwards compatibility. Signed-off-by: Michael Niewöhner <[email protected]>
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
Dec 24, 2019
$fs used with the wrong sed command where should be $mntpnt instead to match a variable exported by read_mtab() The fix is mostly to reuse the sed command found in read_mtab() Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Michael Niewöhner <[email protected]> Signed-off-by: Alexey Smirnoff <[email protected]> Closes openzfs#9168
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
Dec 27, 2019
$fs used with the wrong sed command where should be $mntpnt instead to match a variable exported by read_mtab() The fix is mostly to reuse the sed command found in read_mtab() Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Michael Niewöhner <[email protected]> Signed-off-by: Alexey Smirnoff <[email protected]> Closes openzfs#9168
tonyhutter
pushed a commit
that referenced
this pull request
Jan 23, 2020
$fs used with the wrong sed command where should be $mntpnt instead to match a variable exported by read_mtab() The fix is mostly to reuse the sed command found in read_mtab() Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Michael Niewöhner <[email protected]> Signed-off-by: Alexey Smirnoff <[email protected]> Closes #9168
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
$fsused with the wrong sed command where should be$mntpntinstead to matcha variable exported by
read_mtab()The fix is mostly to reuse the sed command found in
read_mtab()Signed-off-by: Alexey Smirnoff [email protected]
Motivation and Context
I need the working
in_mtab()functionto be able to implement rw remount in
/etc/init.d/zfs-mountDescription
How Has This Been Tested?
Types of changes
Checklist:
Signed-off-by.