Merge pull request #1699 from stschulte/ticket/master/mount_fixes

Various mount fixes

Author: adrienthebo

lib/puppet/provider/mount.rb

@@ -5,10 +5,15 @@
 module Puppet::Provider::Mount
   # This only works when the mount point is synced to the fstab.
   def mount
-    # Manually pass the mount options in, since some OSes *cough*OS X*cough* don't
-    # read from /etc/fstab but still want to use this type.
     args = []
-    args << "-o" << self.options if self.options and self.options != :absent
+
+    # In general we do not have to pass mountoptions because we always
+    # flush /etc/fstab before attempting to mount. But old code suggests
+    # that MacOS always needs the mount options to be explicitly passed to
+    # the mount command
+    if Facter.value(:kernel) == 'Darwin'
+      args << "-o" << self.options if self.options and self.options != :absent
+    end
     args << resource[:name]
 
     mountcmd(*args)

lib/puppet/type/mount.rb

@@ -118,6 +118,10 @@ def syncothers
         device is supporting by the mount, including network
         devices or devices specified by UUID rather than device
         path, depending on the operating system."
+
+      validate do |value|
+        raise Puppet::Error, "device must not contain whitespace: #{value}" if value =~ /\s/
+      end
     end
 
     # Solaris specifies two devices, not just one.
@@ -128,47 +132,68 @@ def syncothers
 
       # Default to the device but with "dsk" replaced with "rdsk".
       defaultto do
-        if Facter["osfamily"].value == "Solaris"
-          device = @resource.value(:device)
-          if device =~ %r{/dsk/}
+        if Facter.value(:osfamily) == "Solaris"
+          if device = resource[:device] and device =~ %r{/dsk/}
             device.sub(%r{/dsk/}, "/rdsk/")
+          elsif fstype = resource[:fstype] and fstype == 'nfs'
+            '-'
           else
             nil
           end
         else
           nil
         end
       end
+
+      validate do |value|
+        raise Puppet::Error, "blockdevice must not contain whitespace: #{value}" if value =~ /\s/
+      end
     end
 
     newproperty(:fstype) do
       desc "The mount type.  Valid values depend on the
         operating system.  This is a required option."
+
+      validate do |value|
+        raise Puppet::Error, "fstype must not contain whitespace: #{value}" if value =~ /\s/
+      end
     end
 
     newproperty(:options) do
       desc "Mount options for the mounts, as they would
         appear in the fstab."
+
+      validate do |value|
+        raise Puppet::Error, "option must not contain whitespace: #{value}" if value =~ /\s/
+      end
     end
 
     newproperty(:pass) do
       desc "The pass in which the mount is checked."
 
       defaultto {
-        0 if @resource.managed?
+        if @resource.managed?
+          if Facter.value(:osfamily) == 'Solaris'
+            '-'
+          else
+            0
+          end
+        end
       }
     end
 
     newproperty(:atboot) do
       desc "Whether to mount the mount at boot.  Not all platforms
         support this."
+
+      newvalues :yes, :no
     end
 
     newproperty(:dump) do
       desc "Whether to dump the mount.  Not all platform support this.
         Valid values are `1` or `0`. or `2` on FreeBSD, Default is `0`."
 
-      if Facter["operatingsystem"].value == "FreeBSD"
+      if Facter.value(:operatingsystem) == "FreeBSD"
         newvalue(%r{(0|1|2)})
       else
         newvalue(%r{(0|1)})
@@ -197,6 +222,14 @@ def syncothers
       desc "The mount path for the mount."
 
       isnamevar
+
+      validate do |value|
+        raise Puppet::Error, "name must not contain whitespace: #{value}" if value =~ /\s/
+      end
+
+      munge do |value|
+        value.gsub(/^(.+?)\/*$/, '\1')
+      end
     end
 
     newparam(:remounts) do

spec/integration/provider/mount_spec.rb

@@ -20,6 +20,7 @@ def create_fake_fstab(initially_contains_entry)
     @current_options = "local"
     @current_device = "/dev/disk1s1"
     Puppet::Type.type(:mount).defaultprovider.stubs(:default_target).returns(@fake_fstab)
+    Facter.stubs(:value).with(:kernel).returns('Darwin')
     Facter.stubs(:value).with(:operatingsystem).returns('Darwin')
     Facter.stubs(:value).with(:osfamily).returns('Darwin')
     Puppet::Util::ExecutionStub.set do |command, options|

spec/unit/provider/mount/parsed_spec.rb

@@ -2,114 +2,102 @@
 require 'spec_helper'
 require 'shared_behaviours/all_parsedfile_providers'
 
-provider_class = Puppet::Type.type(:mount).provider(:parsed)
+describe Puppet::Type.type(:mount).provider(:parsed), :unless => Puppet.features.microsoft_windows? do
 
-describe provider_class, :unless => Puppet.features.microsoft_windows? do
+  let :vfstab_sample do
+    "/dev/dsk/c0d0s0 /dev/rdsk/c0d0s0 \t\t    /  \t    ufs     1 no\t-"
+  end
 
-  before :each do
-    @mount_class = Puppet::Type.type(:mount)
-    @provider = @mount_class.provider(:parsed)
+  let :fstab_sample do
+    "/dev/vg00/lv01\t/spare   \t  \t   ext3    defaults\t1 2"
   end
 
   # LAK:FIXME I can't mock Facter because this test happens at parse-time.
   it "should default to /etc/vfstab on Solaris" do
     pending "This test only works on Solaris" unless Facter.value(:osfamily) == 'Solaris'
-    Puppet::Type.type(:mount).provider(:parsed).default_target.should == '/etc/vfstab'
+    described_class.default_target.should == '/etc/vfstab'
   end
 
   it "should default to /etc/fstab on anything else" do
     pending "This test does not work on Solaris" if Facter.value(:osfamily) == 'Solaris'
-    Puppet::Type.type(:mount).provider(:parsed).default_target.should == '/etc/fstab'
+    described_class.default_target.should == '/etc/fstab'
   end
 
   describe "when parsing a line" do
-
     it "should not crash on incomplete lines in fstab" do
-      parse = @provider.parse <<-FSTAB
+      parse = described_class.parse <<-FSTAB
 /dev/incomplete
 /dev/device       name
 FSTAB
-      lambda{ @provider.to_line(parse[0]) }.should_not raise_error
+      expect { described_class.to_line(parse[0]) }.to_not raise_error
     end
 
 #   it_should_behave_like "all parsedfile providers",
 #     provider_class, my_fixtures('*.fstab')
 
     describe "on Solaris", :if => Facter.value(:osfamily) == 'Solaris' do
-
-      before :each do
-        @example_line = "/dev/dsk/c0d0s0 /dev/rdsk/c0d0s0 \t\t    /  \t    ufs     1 no\t-"
-      end
-
       it "should extract device from the first field" do
-        @provider.parse_line(@example_line)[:device].should == '/dev/dsk/c0d0s0'
+        described_class.parse_line(vfstab_sample)[:device].should == '/dev/dsk/c0d0s0'
       end
 
       it "should extract blockdevice from second field" do
-        @provider.parse_line(@example_line)[:blockdevice].should == "/dev/rdsk/c0d0s0"
+        described_class.parse_line(vfstab_sample)[:blockdevice].should == "/dev/rdsk/c0d0s0"
       end
 
       it "should extract name from third field" do
-        @provider.parse_line(@example_line)[:name].should == "/"
+        described_class.parse_line(vfstab_sample)[:name].should == "/"
       end
 
       it "should extract fstype from fourth field" do
-        @provider.parse_line(@example_line)[:fstype].should == "ufs"
+        described_class.parse_line(vfstab_sample)[:fstype].should == "ufs"
       end
 
       it "should extract pass from fifth field" do
-        @provider.parse_line(@example_line)[:pass].should == "1"
+        described_class.parse_line(vfstab_sample)[:pass].should == "1"
       end
 
       it "should extract atboot from sixth field" do
-        @provider.parse_line(@example_line)[:atboot].should == "no"
+        described_class.parse_line(vfstab_sample)[:atboot].should == "no"
       end
 
       it "should extract options from seventh field" do
-        @provider.parse_line(@example_line)[:options].should == "-"
+        described_class.parse_line(vfstab_sample)[:options].should == "-"
       end
-
     end
 
     describe "on other platforms than Solaris", :if => Facter.value(:osfamily) != 'Solaris' do
-
-      before :each do
-        @example_line = "/dev/vg00/lv01\t/spare   \t  \t   ext3    defaults\t1 2"
-      end
-
       it "should extract device from the first field" do
-        @provider.parse_line(@example_line)[:device].should == '/dev/vg00/lv01'
+        described_class.parse_line(fstab_sample)[:device].should == '/dev/vg00/lv01'
       end
 
       it "should extract name from second field" do
-        @provider.parse_line(@example_line)[:name].should == "/spare"
+        described_class.parse_line(fstab_sample)[:name].should == "/spare"
       end
 
       it "should extract fstype from third field" do
-        @provider.parse_line(@example_line)[:fstype].should == "ext3"
+        described_class.parse_line(fstab_sample)[:fstype].should == "ext3"
       end
 
       it "should extract options from fourth field" do
-        @provider.parse_line(@example_line)[:options].should == "defaults"
+        described_class.parse_line(fstab_sample)[:options].should == "defaults"
       end
 
       it "should extract dump from fifth field" do
-        @provider.parse_line(@example_line)[:dump].should == "1"
+        described_class.parse_line(fstab_sample)[:dump].should == "1"
       end
 
       it "should extract options from sixth field" do
-        @provider.parse_line(@example_line)[:pass].should == "2"
+        described_class.parse_line(fstab_sample)[:pass].should == "2"
       end
-
     end
 
   end
 
   describe "mountinstances" do
     it "should get name from mountoutput found on Solaris" do
       Facter.stubs(:value).with(:osfamily).returns 'Solaris'
-      @provider.stubs(:mountcmd).returns(File.read(my_fixture('solaris.mount')))
-      mounts = @provider.mountinstances
+      described_class.stubs(:mountcmd).returns(File.read(my_fixture('solaris.mount')))
+      mounts = described_class.mountinstances
       mounts.size.should == 6
       mounts[0].should == { :name => '/', :mounted => :yes }
       mounts[1].should == { :name => '/proc', :mounted => :yes }
@@ -121,8 +109,8 @@
 
     it "should get name from mountoutput found on HP-UX" do
       Facter.stubs(:value).with(:osfamily).returns 'HP-UX'
-      @provider.stubs(:mountcmd).returns(File.read(my_fixture('hpux.mount')))
-      mounts = @provider.mountinstances
+      described_class.stubs(:mountcmd).returns(File.read(my_fixture('hpux.mount')))
+      mounts = described_class.mountinstances
       mounts.size.should == 17
       mounts[0].should == { :name => '/', :mounted => :yes }
       mounts[1].should == { :name => '/devices', :mounted => :yes }
@@ -145,8 +133,8 @@
 
     it "should get name from mountoutput found on Darwin" do
       Facter.stubs(:value).with(:osfamily).returns 'Darwin'
-      @provider.stubs(:mountcmd).returns(File.read(my_fixture('darwin.mount')))
-      mounts = @provider.mountinstances
+      described_class.stubs(:mountcmd).returns(File.read(my_fixture('darwin.mount')))
+      mounts = described_class.mountinstances
       mounts.size.should == 6
       mounts[0].should == { :name => '/', :mounted => :yes }
       mounts[1].should == { :name => '/dev', :mounted => :yes }
@@ -158,8 +146,8 @@
 
     it "should get name from mountoutput found on Linux" do
       Facter.stubs(:value).with(:osfamily).returns 'Gentoo'
-      @provider.stubs(:mountcmd).returns(File.read(my_fixture('linux.mount')))
-      mounts = @provider.mountinstances
+      described_class.stubs(:mountcmd).returns(File.read(my_fixture('linux.mount')))
+      mounts = described_class.mountinstances
       mounts[0].should == { :name => '/', :mounted => :yes }
       mounts[1].should == { :name => '/lib64/rc/init.d', :mounted => :yes }
       mounts[2].should == { :name => '/sys', :mounted => :yes }
@@ -169,8 +157,8 @@
 
     it "should get name from mountoutput found on AIX" do
       Facter.stubs(:value).with(:osfamily).returns 'AIX'
-      @provider.stubs(:mountcmd).returns(File.read(my_fixture('aix.mount')))
-      mounts = @provider.mountinstances
+      described_class.stubs(:mountcmd).returns(File.read(my_fixture('aix.mount')))
+      mounts = described_class.mountinstances
       mounts[0].should == { :name => '/', :mounted => :yes }
       mounts[1].should == { :name => '/tmp', :mounted => :yes }
       mounts[2].should == { :name => '/home', :mounted => :yes }
@@ -179,8 +167,8 @@
     end
 
     it "should raise an error if a line is not understandable" do
-      @provider.stubs(:mountcmd).returns("bazinga!")
-      lambda { @provider.mountinstances }.should raise_error Puppet::Error
+      described_class.stubs(:mountcmd).returns("bazinga!")
+      expect { described_class.mountinstances }.to raise_error Puppet::Error, 'Could not understand line bazinga! from mount output'
     end
 
   end
@@ -203,16 +191,16 @@
         # Stub the mount output to our fixture.
         begin
           mount = my_fixture(platform + '.mount')
-          @provider.stubs(:mountcmd).returns File.read(mount)
+          described_class.stubs(:mountcmd).returns File.read(mount)
         rescue
           pending "is #{platform}.mount missing at this point?"
         end
 
         # Note: we have to stub default_target before creating resources
         # because it is used by Puppet::Type::Mount.new to populate the
         # :target property.
-        @provider.stubs(:default_target).returns fstab
-        @retrieve = @provider.instances.collect { |prov| {:name => prov.get(:name), :ensure => prov.get(:ensure)}}
+        described_class.stubs(:default_target).returns fstab
+        @retrieve = described_class.instances.collect { |prov| {:name => prov.get(:name), :ensure => prov.get(:ensure)}}
       end
 
       # Following mountpoint are present in all fstabs/mountoutputs
@@ -246,15 +234,15 @@
         # Stub the mount output to our fixture.
         begin
           mount = my_fixture(platform + '.mount')
-          @provider.stubs(:mountcmd).returns File.read(mount)
+          described_class.stubs(:mountcmd).returns File.read(mount)
         rescue
           pending "is #{platform}.mount missing at this point?"
         end
 
         # Note: we have to stub default_target before creating resources
         # because it is used by Puppet::Type::Mount.new to populate the
         # :target property.
-        @provider.stubs(:default_target).returns fstab
+        described_class.stubs(:default_target).returns fstab
 
         @res_ghost = Puppet::Type::Mount.new(:name => '/ghost')    # in no fake fstab
         @res_mounted = Puppet::Type::Mount.new(:name => '/')       # in every fake fstab
@@ -270,24 +258,24 @@
 
       it "should set :ensure to :unmounted if found in fstab but not mounted" do
         pending("Solaris:Unable to stub Operating System Fact at runtime", :if => Facter.value(:osfamily) == "Solaris")
-        @provider.prefetch(@resource_hash)
+        described_class.prefetch(@resource_hash)
         @res_unmounted.provider.get(:ensure).should == :unmounted
       end
 
       it "should set :ensure to :ghost if not found in fstab but mounted" do
         pending("Solaris:Unable to stub Operating System Fact at runtime", :if => Facter.value(:osfamily) == "Solaris")
-        @provider.prefetch(@resource_hash)
+        described_class.prefetch(@resource_hash)
         @res_ghost.provider.get(:ensure).should == :ghost
       end
 
       it "should set :ensure to :mounted if found in fstab and mounted" do
         pending("Solaris:Unable to stub Operating System Fact at runtime", :if => Facter.value(:osfamily) == "Solaris")
-        @provider.prefetch(@resource_hash)
+        described_class.prefetch(@resource_hash)
         @res_mounted.provider.get(:ensure).should == :mounted
       end
 
       it "should set :ensure to :absent if not found in fstab and not mounted" do
-        @provider.prefetch(@resource_hash)
+        described_class.prefetch(@resource_hash)
         @res_absent.provider.get(:ensure).should == :absent
       end
     end