mount resource: fix for Device-/Sharenames and Mountpoints including … #2257

mgrobelin · 2017-10-18T14:40:20Z

…whitespaces

Device-/Sharenames and Mountpoints on Linux may include whitespaces (\040), e.g. /etc/fstab entry like:

//fileserver.corp.internal/Research\040&\040Development /mnt/Research\040&\040Development cifs OTHER_OPTS

... results in a mount line like:

//fileserver.corp.internal/Research & Development on /mnt/Research & Development type cifs (OTHER_OPTS)

The Linux mount command replaces \040 with whitspace automatically, so this should be tributed.

I used a control like this:

    describe mount('/mnt/Research & Development') do
      it { should be_mounted }
      its('device') { should eq  '//fileserver.corp.internal/Research & Development' }
    end

Before:

  ×  whitespaces-1: Mount with whitespace within sharename and mountpoint. (1 failed)
     ✔  Mount /mnt/Research & Development should be mounted
     ×  Mount /mnt/Research & Development device should eq "//fileserver.corp.internal/Research & Development"

     expected: "//fileserver.corp.internal/Research & Development"
          got: "//fileserver.corp.internal/operational/Research"

     (compared using ==)

After:

  ✔  whitespaces-01: Mount with whitespace within sharename and mountpoint.
     ✔  Mount /mnt/Research & Development should be mounted
     ✔  Mount /mnt/Research & Development device should eq "//fileserver.corp.internal/Research & Development"

Signed-off-by: Markus Grobelin [email protected]

mgrobelin · 2017-10-18T14:46:42Z

Please note, this PR do not address the case, when the Share-/Devicename and/or Mountpoint includes words like on or type, e.g.: a /etc/fstab entry like:

/dev/sda1 /mnt/type\040on\040fire xfs [...]

Will most likely render wrong results / unexpected behavoir.

…whitespaces Device-/Sharenames and Mountpoints on Linux may include whitespaces (\040), e.g. /etc/fstab entry like: ```//fileserver.corp.internal/Research\040&\040Development /mnt/Research\040&\040Development cifs OTHER_OPTS``` ... results in a mount line like: ```//fileserver.corp.internal/Research & Development on /mnt/Research & Development type cifs (OTHER_OPTS)``` The Linux mount command replaces \040 with whitspace automatically, so this should be tributed. I used a control like this: ``` describe mount('/mnt/Research & Development') do it { should be_mounted } its('device') { should eq '//fileserver.corp.internal/Research & Development' } end ``` Before: ``` × whitespaces-1: Mount with whitespace within sharename and mountpoint. (1 failed) ✔ Mount /mnt/Research & Development should be mounted × Mount /mnt/Research & Development device should eq "//fileserver.corp.internal/Research & Development" expected: "//fileserver.corp.internal/Research & Development" got: "//fileserver.corp.internal/Research" (compared using ==) ``` After: ``` ✔ whitespaces-01: Mount with whitespace within sharename and mountpoint. ✔ Mount /mnt/Research & Development should be mounted ✔ Mount /mnt/Research & Development device should eq "//fileserver.corp.internal/Research & Development" ``` Signed-off-by: Markus Grobelin <[email protected]>

Signed-off-by: Markus Grobelin <[email protected]>

adamleff

Looking pretty good! Just a couple of code-cleanup items and a suggestion on a potential way around the "type" being in device/share names.

adamleff · 2017-10-24T21:33:41Z

lib/utils/parser.rb

+      other_opts = type_split[1]
+      fs, path = fs_path.match(%r{^(.+?)\son\s(/.+?)$}).captures
+      mount = [fs, 'on', path, 'type']
+      mount.concat other_opts.scan(/\S+/)


We try to standardize on wrapping all method arguments in parentheses. Can you change this to:

mount.concat(other_opts.scan(/\S+/))

... so it's clearer? Thanks!

adamleff · 2017-10-24T21:34:11Z

lib/utils/parser.rb

+  # Device-/Sharename or Mountpoint includes whitespaces?
+  def includes_whitespaces?(mount_line)
+    ws = mount_line.match(/^(.+)\son\s(.+)\stype\s.*$/)
+    ws.captures[0].include? ' ' or ws.captures[1].include? ' '


Same here.. each of the include? method calls should wrap their arguments with parentheses, please. :)

adamleff · 2017-10-24T21:36:59Z

lib/utils/parser.rb

@@ -67,7 +67,19 @@ module LinuxMountParser
  # this parses the output of mount command (only tested on linux)
  # this method expects only one line of the mount output
  def parse_mount_options(mount_line, compatibility = false)
-    mount = mount_line.scan(/\S+/)
+    if includes_whitespaces? mount_line


As with the other review comments, please wrap arguments with parentheses:

if includes_whitespaces?(mount_line)

adamleff · 2017-10-24T21:43:50Z

lib/utils/parser.rb

+    if includes_whitespaces? mount_line
+      # Device-/Sharenames and Mountpoints including whitespaces require special treatment:
+      # We use the keyword ' type ' to split up and rebuild the desired array of fields
+      type_split = mount_line.split(' type ')


You might be able to use rindex to get the right-most type to avoid picking up a share name with the word type in it with spaces:

type_index = mount_line.rindex(' type ') fs_path = mount_line[0...type_index] other_opts_index = type_index + 6 # since ' type ' is 6 chars other_opts = mount_line[other_opts_index..-1] # ... etc ...

I think this might decently well, but would need to be tested. Worth playing around with a bit?

Definitely worth, but I left it out for now due to high complexity. Maybe later, but I cannot promise due to endofyear-crunch.

If one would address this edge-case, it might be clever to do the same for BsdMountParser, right? What would be your suggestion to keep it DRY?

I'm OK leaving it out for this PR and we can circle back to it if someone raises a bug or if you feel like coming back for a second iteration. :)

I would definitely DRY it up if possible as long as it doesn't make it too complicated. I'm happy to help as needed!

…chef/inspec/pull/2257/files Signed-off-by: Markus Grobelin <[email protected]>

adamleff

@mgrobelin just one more set of parentheses was missed in your last commit from my initial review, and then I think we can move forward with this. Thanks!

adamleff · 2017-10-26T21:52:11Z

lib/utils/parser.rb

@@ -67,7 +67,19 @@ module LinuxMountParser
  # this parses the output of mount command (only tested on linux)
  # this method expects only one line of the mount output
  def parse_mount_options(mount_line, compatibility = false)
-    mount = mount_line.scan(/\S+/)
+    if includes_whitespaces? mount_line


Missed one set of parentheses here and then I think we're good!

…whitespaces Signed-off-by: Markus Grobelin <[email protected]>

adamleff

Thanks for your work on this fix, @mgrobelin!

chris-rock

Thank you @mgrobelin and @adamleff

mgrobelin requested a review from a team as a code owner October 18, 2017 14:40

mgrobelin added 2 commits October 19, 2017 13:58

mounts_with_whitespaces: make lint happy

d4a9158

Signed-off-by: Markus Grobelin <[email protected]>

adamleff suggested changes Oct 24, 2017

View reviewed changes

adamleff added the Type: Bug Feature not working as expected label Oct 24, 2017

mount resource: added parentheses as suggested by https://github.com/…

2750ff8

…chef/inspec/pull/2257/files Signed-off-by: Markus Grobelin <[email protected]>

adamleff suggested changes Oct 26, 2017

View reviewed changes

mount resource: fix for Device-/Sharenames and Mountpoints including …

8d2ee6e

…whitespaces Signed-off-by: Markus Grobelin <[email protected]>

adamleff approved these changes Oct 31, 2017

View reviewed changes

adamleff requested a review from a team October 31, 2017 20:39

chris-rock approved these changes Nov 1, 2017

View reviewed changes

chris-rock merged commit 221db7e into inspec:master Nov 1, 2017

mgrobelin deleted the mounts_with_whitespaces branch November 1, 2017 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mount resource: fix for Device-/Sharenames and Mountpoints including … #2257

mount resource: fix for Device-/Sharenames and Mountpoints including … #2257

mgrobelin commented Oct 18, 2017 •

edited

Loading

mgrobelin commented Oct 18, 2017

adamleff left a comment

adamleff Oct 24, 2017

adamleff Oct 24, 2017

adamleff Oct 24, 2017

adamleff Oct 24, 2017

mgrobelin Oct 26, 2017

adamleff Oct 26, 2017

adamleff left a comment

adamleff Oct 26, 2017

adamleff left a comment

chris-rock left a comment

mount resource: fix for Device-/Sharenames and Mountpoints including … #2257

mount resource: fix for Device-/Sharenames and Mountpoints including … #2257

Conversation

mgrobelin commented Oct 18, 2017 • edited Loading

mgrobelin commented Oct 18, 2017

adamleff left a comment

Choose a reason for hiding this comment

adamleff Oct 24, 2017

Choose a reason for hiding this comment

adamleff Oct 24, 2017

Choose a reason for hiding this comment

adamleff Oct 24, 2017

Choose a reason for hiding this comment

adamleff Oct 24, 2017

Choose a reason for hiding this comment

mgrobelin Oct 26, 2017

Choose a reason for hiding this comment

adamleff Oct 26, 2017

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

adamleff Oct 26, 2017

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

mgrobelin commented Oct 18, 2017 •

edited

Loading