Ruby has a standard Tempfile class dedicated to safe temporary file management; take advantage of that in the File type rather that building our own version of the same.
Also, update the spec tests to reflect changes in the implementation. Signed-off-by: Daniel Pittman <[email protected]> --- lib/puppet/type/file.rb | 51 +++++++++++++++++++++---------------------- spec/unit/type/file_spec.rb | 5 +-- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb index f35a264..88a0ccb 100644 --- a/lib/puppet/type/file.rb +++ b/lib/puppet/type/file.rb @@ -3,6 +3,7 @@ require 'cgi' require 'etc' require 'uri' require 'fileutils' +require 'tempfile' require 'puppet/network/handler' require 'puppet/util/diff' require 'puppet/util/checksums' @@ -708,42 +709,46 @@ Puppet::Type.newtype(:file) do def write(property) remove_existing(:file) - use_temporary_file = write_temporary_file? - if use_temporary_file - path = "#{self[:path]}.puppettmp_#{rand(10000)}" - path = "#{self[:path]}.puppettmp_#{rand(10000)}" while File.exists?(path) or File.symlink?(path) - else - path = self[:path] - end + path = self[:path] + name = File.basename(path) + dir = File.dirname(path) mode = self.should(:mode) # might be nil umask = mode ? 000 : 022 - content_checksum = Puppet::Util.withumask(umask) { File.open(path, 'w', mode) { |f| write_content(f) } } - - # And put our new file in place - if use_temporary_file # This is only not true when our file is empty. + Puppet::Util.withumask(umask) { + # Write the content to a temporary file. begin - fail_if_checksum_is_wrong(path, content_checksum) if validate_checksum? - File.rename(path, self[:path]) + Tempfile.open(name, dir) { |f| + begin + checksum = write_content(f) + + # Ensure we have pushed that content safely to disk. + f.flush; f.fsync + + # Make sure the content checksum is right... + fail_if_checksum_is_wrong(f.path, checksum) if validate_checksum? + + # ...and perform an atomic replace of the old file. + File.rename(f.path, path) + rescue => detail + fail "Could not rename temporary file #{f.path} to #{path}: #{detail}" + end + } rescue => detail - fail "Could not rename temporary file #{path} to #{self[:path]}: #{detail}" - ensure - # Make sure the created file gets removed - File.unlink(path) if FileTest.exists?(path) + fail "Could not create temporary file for #{path}: #{detail}" end - end + } # make sure all of the modes are actually correct property_fix - end private # Should we validate the checksum of the file we're writing? def validate_checksum? - self[:checksum] !~ /time/ + property(:content) and self[:checksum] !~ /time/ end # Make sure the file we wrote out is what we think it is. @@ -763,12 +768,6 @@ Puppet::Type.newtype(:file) do private - def write_temporary_file? - # unfortunately we don't know the source file size before fetching it - # so let's assume the file won't be empty - (c = property(:content) and c.length) || (s = @parameters[:source] and 1) - end - # There are some cases where all of the work does not get done on # file creation/modification, so we have to do some extra checking. def property_fix diff --git a/spec/unit/type/file_spec.rb b/spec/unit/type/file_spec.rb index 7d93dfd..b2d9ef1 100755 --- a/spec/unit/type/file_spec.rb +++ b/spec/unit/type/file_spec.rb @@ -40,9 +40,8 @@ describe Puppet::Type.type(:file) do describe "#write" do it "should propagate failures encountered when renaming the temporary file" do - File.stubs(:open) + Tempfile.expects(:open).raises ArgumentError - File.expects(:rename).raises ArgumentError file = Puppet::Type::File.new(:name => "/my/file", :backup => "puppet") file.stubs(:validate_checksum?).returns(false) @@ -55,7 +54,7 @@ describe Puppet::Type.type(:file) do it "should delegate writing to the content property" do filehandle = stub_everything 'fh' - File.stubs(:open).yields(filehandle) + Tempfile.stubs(:open).yields(filehandle) File.stubs(:rename) property = stub('content_property', :actual_content => "something", :length => "something".length) file = Puppet::Type::File.new(:name => "/my/file", :backup => "puppet") -- 1.7.1 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
