From 171f4184c0a698c4a6b05767accb6002ffcd18a0 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 27 Sep 2010 18:19:19 -0700 Subject: [PATCH] Instead of using Kernel#system, use custom piped solution --- CHANGELOG.md | 3 ++- lib/vagrant/hosts/bsd.rb | 17 ++++++++++------ lib/vagrant/hosts/linux.rb | 17 ++++++++++------ lib/vagrant/util.rb | 1 + lib/vagrant/util/sh.rb | 33 ++++++++++++++++++++++++++++++++ test/vagrant/hosts/bsd_test.rb | 18 ++++++++++------- test/vagrant/hosts/linux_test.rb | 18 ++++++++++------- test/vagrant/util/sh_test.rb | 22 +++++++++++++++++++++ 8 files changed, 102 insertions(+), 27 deletions(-) create mode 100644 lib/vagrant/util/sh.rb create mode 100644 test/vagrant/util/sh_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index f99d89d6c..ea1279b4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ ## 0.6.4 (unreleased) - + - Replaced `Kernel#system` calls with custom `Vagrant::Util::Sh` method to + fix strange error issues. ## 0.6.1, 0.6.2, 0.6.3 (September 27, 2010) diff --git a/lib/vagrant/hosts/bsd.rb b/lib/vagrant/hosts/bsd.rb index 9bf3f2310..1fee8f48b 100644 --- a/lib/vagrant/hosts/bsd.rb +++ b/lib/vagrant/hosts/bsd.rb @@ -4,10 +4,15 @@ module Vagrant class BSD < Base include Util include Util::Retryable + include Util::Sh def nfs? retryable(:tries => 10, :on => TypeError) do - system("which nfsd > /dev/null 2>&1") + _, status = sh("which nfsd") + + # Sometimes the status is nil for some reason. In that case, force a retry + raise TypeError.new("Bad status code") if !status + status.success? end end @@ -25,22 +30,22 @@ module Vagrant output.split("\n").each do |line| # This should only ask for administrative permission once, even # though its executed in multiple subshells. - system(%Q[sudo su root -c "echo '#{line}' >> /etc/exports"]) + sh(%Q[sudo su root -c "echo '#{line}' >> /etc/exports"]) end # We run restart here instead of "update" just in case nfsd # is not starting - system("sudo nfsd restart") + sh("sudo nfsd restart") end def nfs_cleanup return if !File.exist?("/etc/exports") - system("cat /etc/exports | grep 'VAGRANT-BEGIN: #{env.vm.uuid}' > /dev/null 2>&1") + _, status = sh("cat /etc/exports | grep 'VAGRANT-BEGIN: #{env.vm.uuid}'") - if $?.to_i == 0 + if status.success? # Use sed to just strip out the block of code which was inserted # by Vagrant - system("sudo sed -e '/^# VAGRANT-BEGIN: #{env.vm.uuid}/,/^# VAGRANT-END: #{env.vm.uuid}/ d' -i bak /etc/exports") + sh("sudo sed -e '/^# VAGRANT-BEGIN: #{env.vm.uuid}/,/^# VAGRANT-END: #{env.vm.uuid}/ d' -i bak /etc/exports") end end end diff --git a/lib/vagrant/hosts/linux.rb b/lib/vagrant/hosts/linux.rb index 68466491e..12523f298 100644 --- a/lib/vagrant/hosts/linux.rb +++ b/lib/vagrant/hosts/linux.rb @@ -4,11 +4,16 @@ module Vagrant class Linux < Base include Util include Util::Retryable + include Util::Sh def nfs? retryable(:tries => 10, :on => TypeError) do # Check procfs to see if NFSd is a supported filesystem - system("cat /proc/filesystems | grep nfsd > /dev/null 2>&1") + _, status = sh("cat /proc/filesystems | grep nfsd") + + # Sometimes the status is nil for some reason. In that case, force a retry + raise TypeError.new("Bad status code") if !status + status.success? end end @@ -24,22 +29,22 @@ module Vagrant output.split("\n").each do |line| # This should only ask for administrative permission once, even # though its executed in multiple subshells. - system(%Q[sudo su root -c "echo '#{line}' >> /etc/exports"]) + sh(%Q[sudo su root -c "echo '#{line}' >> /etc/exports"]) end # We run restart here instead of "update" just in case nfsd # is not starting - system("sudo /etc/init.d/nfs-kernel-server restart") + sh("sudo /etc/init.d/nfs-kernel-server restart") end def nfs_cleanup return if !File.exist?("/etc/exports") - system("cat /etc/exports | grep 'VAGRANT-BEGIN: #{env.vm.uuid}' > /dev/null 2>&1") + _, status = sh("cat /etc/exports | grep 'VAGRANT-BEGIN: #{env.vm.uuid}'") - if $?.to_i == 0 + if status.success? # Use sed to just strip out the block of code which was inserted # by Vagrant - system("sudo sed -e '/^# VAGRANT-BEGIN: #{env.vm.uuid}/,/^# VAGRANT-END: #{env.vm.uuid}/ d' -ibak /etc/exports") + sh("sudo sed -e '/^# VAGRANT-BEGIN: #{env.vm.uuid}/,/^# VAGRANT-END: #{env.vm.uuid}/ d' -ibak /etc/exports") end end end diff --git a/lib/vagrant/util.rb b/lib/vagrant/util.rb index bcfb27841..847bb16e5 100644 --- a/lib/vagrant/util.rb +++ b/lib/vagrant/util.rb @@ -7,6 +7,7 @@ module Vagrant autoload :Platform, 'vagrant/util/platform' autoload :ResourceLogger, 'vagrant/util/resource_logger' autoload :Retryable, 'vagrant/util/retryable' + autoload :Sh, 'vagrant/util/sh' autoload :StackedProcRunner, 'vagrant/util/stacked_proc_runner' autoload :TemplateRenderer, 'vagrant/util/template_renderer' end diff --git a/lib/vagrant/util/sh.rb b/lib/vagrant/util/sh.rb new file mode 100644 index 000000000..23181e2b4 --- /dev/null +++ b/lib/vagrant/util/sh.rb @@ -0,0 +1,33 @@ +module Vagrant + module Util + module Sh + # Runs the given shell command, collecting STDERR and STDOUT + # into a single string and returning it. After this method, + # `$?` will be set to a `Process::Status` object. + # + # @param [String] command Command to run + # @return [Array] An array of `[output, status]` where status is `Process::Status` + def sh(command, *args) + # Use a pipe to execute the given command + rd, wr = IO.pipe + pid = fork do + rd.close + $stdout.reopen wr + $stderr.reopen wr + exec(command, *args) rescue nil + exit! 1 # Should never reach this point + end + + # Close our end of the writer pipe, read the output until + # its over, and wait for the process to end. + wr.close + out = "" + out << rd.read until rd.eof? + _, status = Process.wait2(-1, Process::WNOHANG) + + # Finally, return the result + [out, status] + end + end + end +end diff --git a/test/vagrant/hosts/bsd_test.rb b/test/vagrant/hosts/bsd_test.rb index 8e73d15c8..1539e8ba4 100644 --- a/test/vagrant/hosts/bsd_test.rb +++ b/test/vagrant/hosts/bsd_test.rb @@ -9,26 +9,30 @@ class BSDHostTest < Test::Unit::TestCase context "supporting nfs check" do should "support NFS" do - @instance.expects(:system).returns(true) + result = @instance.sh("echo") + @instance.expects(:sh).returns(result) assert @instance.nfs? end should "not support NFS if nfsd is not found" do - @instance.expects(:system).returns(false) + result = @instance.sh("sdalkfjasdlkfj") + @instance.expects(:sh).returns(result) assert !@instance.nfs? end should "retry until a boolean is returned" do + result = @instance.sh("echo") + seq = sequence("seq") - @instance.expects(:system).in_sequence(seq).raises(TypeError.new("foo")) - @instance.expects(:system).in_sequence(seq).returns(true) + @instance.expects(:sh).once.in_sequence(seq).raises(TypeError.new("foo")) + @instance.expects(:sh).once.in_sequence(seq).returns(result) assert @instance.nfs? end end context "nfs export" do setup do - @instance.stubs(:system) + @instance.stubs(:sh) @ip = "foo" @folders = "bar" @@ -41,8 +45,8 @@ class BSDHostTest < Test::Unit::TestCase :ip => @ip, :folders => @folders).returns(output) - @instance.expects(:system).times(output.split("\n").length) - @instance.expects(:system).with("sudo nfsd restart") + @instance.expects(:sh).times(output.split("\n").length) + @instance.expects(:sh).with("sudo nfsd restart") @instance.nfs_export(@ip, @folders) end end diff --git a/test/vagrant/hosts/linux_test.rb b/test/vagrant/hosts/linux_test.rb index 5c1ff9a85..c04a1ed7a 100644 --- a/test/vagrant/hosts/linux_test.rb +++ b/test/vagrant/hosts/linux_test.rb @@ -10,26 +10,30 @@ class LinuxHostTest < Test::Unit::TestCase context "supporting nfs check" do should "support NFS" do - @instance.expects(:system).returns(true) + result = @instance.sh("echo") + @instance.expects(:sh).returns(result) assert @instance.nfs? end should "not support NFS if nfsd is not found" do - @instance.expects(:system).returns(false) + result = @instance.sh("adosifjssdlkfj") + @instance.expects(:sh).returns(result) assert !@instance.nfs? end should "retry until a boolean is returned" do + result = @instance.sh("echo") + seq = sequence("seq") - @instance.expects(:system).in_sequence(seq).raises(TypeError.new("foo")) - @instance.expects(:system).in_sequence(seq).returns(true) + @instance.expects(:sh).once.in_sequence(seq).raises(TypeError.new("foo")) + @instance.expects(:sh).once.in_sequence(seq).returns(result) assert @instance.nfs? end end context "nfs export" do setup do - @instance.stubs(:system) + @instance.stubs(:sh) @ip = "foo" @folders = "bar" @@ -42,8 +46,8 @@ class LinuxHostTest < Test::Unit::TestCase :ip => @ip, :folders => @folders).returns(output) - @instance.expects(:system).times(output.split("\n").length) - @instance.expects(:system).with("sudo /etc/init.d/nfs-kernel-server restart") + @instance.expects(:sh).times(output.split("\n").length) + @instance.expects(:sh).with("sudo /etc/init.d/nfs-kernel-server restart") @instance.nfs_export(@ip, @folders) end end diff --git a/test/vagrant/util/sh_test.rb b/test/vagrant/util/sh_test.rb new file mode 100644 index 000000000..82e9c9f98 --- /dev/null +++ b/test/vagrant/util/sh_test.rb @@ -0,0 +1,22 @@ +require "test_helper" + +class ShUtilTest < Test::Unit::TestCase + setup do + @klass = Class.new do + extend Vagrant::Util::Sh + end + end + + should "execute and return the output" do + out, _ = @klass.sh("echo 'hello'") + assert_equal "hello\n", out + end + + should "populate the exit status variable" do + _, status = @klass.sh("echo") + assert status.success? + + _, status = @klass.sh("sdklfjslkfj") + assert !status.success? + end +end