Include error handling when subprocess commands fail

lib/vagrant/errors.rb

@@ -456,6 +456,10 @@ module Vagrant
       error_key(:nfs_bad_exports)
     end
 
+    class NFSExportsFailed < VagrantError
+      error_key(:nfs_exports_failed)
+    end
+
     class NFSCantReadExports < VagrantError
       error_key(:nfs_cant_read_exports)
     end

plugins/hosts/linux/cap/nfs.rb

@@ -39,7 +39,7 @@ module VagrantPlugins
           sleep 0.5
 
           nfs_cleanup("#{Process.uid} #{id}")
-          output = "#{File.read(NFS_EXPORTS_PATH)}\n#{output}"
+          output = "#{nfs_exports_content}\n#{output}"
           nfs_write_exports(output)
 
           if nfs_running?(nfs_check_command)
@@ -65,7 +65,7 @@ module VagrantPlugins
           user = Process.uid
 
           # Create editor instance for removing invalid IDs
-          editor = Vagrant::Util::StringBlockEditor.new(File.read(NFS_EXPORTS_PATH))
+          editor = Vagrant::Util::StringBlockEditor.new(nfs_exports_content)
 
           # Build composite IDs with UID information and discover invalid entries
           composite_ids = valid_ids.map do |v_id|
@@ -87,7 +87,7 @@ module VagrantPlugins
         def self.nfs_cleanup(remove_ids)
           return if !File.exist?(NFS_EXPORTS_PATH)
 
-          editor = Vagrant::Util::StringBlockEditor.new(File.read(NFS_EXPORTS_PATH))
+          editor = Vagrant::Util::StringBlockEditor.new(nfs_exports_content)
           remove_ids = Array(remove_ids)
 
           # Remove all invalid ID entries
@@ -98,7 +98,7 @@ module VagrantPlugins
         end
 
         def self.nfs_write_exports(new_exports_content)
-          if(File.read(NFS_EXPORTS_PATH).strip != new_exports_content.strip)
+          if(nfs_exports_content != new_exports_content.strip)
             begin
               # Write contents out to temporary file
               new_exports_file = Tempfile.create('vagrant')
@@ -106,7 +106,6 @@ module VagrantPlugins
               new_exports_file.close
               new_exports_path = new_exports_file.path
 
-              # puts File.read(new_exports_path).inspect
               # Only use "sudo" if we can't write to /etc/exports directly
               sudo_command = ""
               sudo_command = "sudo " if !File.writable?(NFS_EXPORTS_PATH)
@@ -117,13 +116,25 @@ module VagrantPlugins
               if existing_stat.mode != new_stat.mode
                 File.chmod(existing_stat.mode, new_exports_path)
               end
-              # TODO: Error check
               if existing_stat.uid != new_stat.uid || existing_stat.gid != new_stat.gid
-                system("#{sudo_command}chown #{existing_stat.uid}:#{existing_stat.gid} #{new_exports_path}")
+                chown_cmd = "#{sudo_command}chown #{existing_stat.uid}:#{existing_stat.gid} #{new_exports_path}"
+                result = Vagrant::Util::Subprocess.execute(*Shellwords.split(chown_cmd))
+                if result.exit_code != 0
+                  raise Vagrant::Errors::NFSExportsFailed,
+                    command: chown_cmd,
+                    stderr: result.stderr,
+                    stdout: result.stdout
+                end
+              end
+              # Always force move the file to prevent overwrite prompting
+              mv_cmd = "#{sudo_command}mv -f #{new_exports_path} #{NFS_EXPORTS_PATH}"
+              result = Vagrant::Util::Subprocess.execute(*Shellwords.split(mv_cmd))
+              if result.exit_code != 0
+                raise Vagrant::Errors::NFSExportsFailed,
+                  command: mv_cmd,
+                  stderr: result.stderr,
+                  stdout: result.stdout
               end
-
-              # Replace existing exports file
-              system("#{sudo_command}mv #{new_exports_path} #{NFS_EXPORTS_PATH}")
             ensure
               if File.exist?(new_exports_path)
                 File.unlink(new_exports_path)
@@ -132,6 +143,27 @@ module VagrantPlugins
           end
         end
 
+        def self.nfs_exports_content
+          if(File.exist?(NFS_EXPORTS_PATH))
+            if(File.readable?(NFS_EXPORTS_PATH))
+              File.read(NFS_EXPORTS_PATH)
+            else
+              cmd = "sudo cat #{NFS_EXPORTS_PATH}"
+              result = Vagrant::Util::Subprocess.execute(*Shellwords.split(cmd))
+              if result.exit_code != 0
+                raise Vagrant::Errors::NFSExportsFailed,
+                  command: cmd,
+                  stderr: result.stderr,
+                  stdout: result.stdout
+              else
+                result.stdout
+              end
+            end
+          else
+            ""
+          end
+        end
+
         def self.nfs_opts_setup(folders)
           folders.each do |k, opts|
             if !opts[:linux__nfs_options]

templates/locales/en.yml

@@ -877,6 +877,14 @@ en:
         the issues below and execute "vagrant reload":
 
         %{output}
+      nfs_exports_failed: |-
+        Vagrant failed to install an updated NFS exports file. This may be
+        due to overly restrictive permissions on your NFS exports file. Please
+        validate them and try again.
+
+        command: %{command}
+        stdout: %{stdout}
+        stderr: %{stderr}
       nfs_cant_read_exports: |-
         Vagrant can't read your current NFS exports! The exports file should be
         readable by any user. This is usually caused by invalid permissions

test/unit/plugins/hosts/linux/cap/nfs_test.rb

@@ -31,20 +31,22 @@ describe VagrantPlugins::HostLinux::Cap::NFS do
     VagrantPlugins::HostLinux::Cap::NFS.const_set(:NFS_EXPORTS_PATH, @original_exports_path)
     File.unlink(tmp_exports_path.to_s) if File.exist?(tmp_exports_path.to_s)
     @tmp_exports = nil
-    @env = nil
   end
 
   describe ".nfs_export" do
+
+    let(:cap){ caps.get(:nfs_export) }
+
     before do
       allow(env).to receive(:host).and_return(host)
       allow(host).to receive(:capability).with(:nfs_apply_command).and_return("/bin/true")
       allow(host).to receive(:capability).with(:nfs_check_command).and_return("/bin/true")
       allow(host).to receive(:capability).with(:nfs_start_command).and_return("/bin/true")
       allow(ui).to receive(:info)
+      allow(cap).to receive(:system).with("sudo /bin/true").and_return(true)
+      allow(cap).to receive(:system).with("/bin/true").and_return(true)
     end
 
-    let(:cap){ caps.get(:nfs_export) }
-
     it "should export new entries" do
       cap.nfs_export(env, ui, SecureRandom.uuid, ["127.0.0.1"], "tmp" => {:hostpath => "/tmp"})
       exports_content = File.read(exports_path)
@@ -106,7 +108,6 @@ EOH
       expect(exports_content).to include("/var")
       expect(exports_content).not_to include("/tmp")
     end
-
   end
 
   describe ".nfs_write_exports" do
@@ -137,8 +138,19 @@ EOH
       expect(original_stat.mode).to eq(updated_stat.mode)
     end
 
+    it "should raise exception when failing to move new exports file" do
+      expect(Vagrant::Util::Subprocess).to receive(:execute).and_return(
+        Vagrant::Util::Subprocess::Result.new(1, "Failed to move file", "")
+      )
+      expect{ described_class.nfs_write_exports("new content") }.to raise_error(Vagrant::Errors::NFSExportsFailed)
+    end
+
     it "should retain existing file owner and group IDs" do
       pending("investigate using a simulated FS to test")
     end
+
+    it "should raise custom exception when chown fails" do
+      pending("investigate using a simulated FS to test")
+    end
   end
 end