@@ -, +, @@ - New file must have binmode set to generate checksums appropriately across operating sytems that results from newline differences - replace_file has been adjusted so that passing a mode is now optional, and mode is only applied when the passed value is not nil - Windows code has been cleaned up / refactored to reduce unnecessary confusing code paths. The simple mechanism for ensuring Windows ReplaceFile will always succeed is to first create a file by touching it (yielding default permissions) if it does not yet exist. - replace_file is now allowed to take a nil `default_mode` so that when the file type is not attempting to manage the mode of the file we will end up with the system default. --- a/lib/puppet/type/file.rb +++ a/lib/puppet/type/file.rb @@ -704,36 +704,25 @@ 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 + assumed_default_mode = 0644 mode = self.should(:mode) # might be nil - umask = mode ? 000 : 022 - mode_int = mode ? symbolic_mode_to_int(mode, 0644) : nil - - content_checksum = Puppet::Util.withumask(umask) { ::File.open(path, 'wb', mode_int ) { |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. - begin - fail_if_checksum_is_wrong(path, content_checksum) if validate_checksum? - ::File.rename(path, self[:path]) - 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) + mode_int = mode ? symbolic_mode_to_int(mode, assumed_default_mode) : nil + + if write_temporary_file? + Puppet::Util.replace_file(self[:path], mode_int) do |file| + file.binmode + content_checksum = write_content(file) + file.flush + fail_if_checksum_is_wrong(file.path, content_checksum) if validate_checksum? end + else + umask = mode ? 000 : 022 + Puppet::Util.withumask(umask) { ::File.open(self[:path], 'wb', mode_int ) { |f| write_content(f) } } end # make sure all of the modes are actually correct property_fix - end private --- a/lib/puppet/util.rb +++ a/lib/puppet/util.rb @@ -404,10 +404,13 @@ module Util def replace_file(file, default_mode, &block) raise Puppet::DevError, "replace_file requires a block" unless block_given? - unless valid_symbolic_mode?(default_mode) - raise Puppet::DevError, "replace_file default_mode: #{default_mode} is invalid" + if default_mode + unless valid_symbolic_mode?(default_mode) + raise Puppet::DevError, "replace_file default_mode: #{default_mode} is invalid" + end + + mode = symbolic_mode_to_int(normalize_symbolic_mode(default_mode)) end - mode = symbolic_mode_to_int(normalize_symbolic_mode(default_mode)) file = Pathname(file) tempfile = Tempfile.new(file.basename.to_s, file.dirname.to_s) @@ -420,16 +423,21 @@ module Util # and specifically handle the platform, which has all sorts of magic. # So, unlike Unix, we don't pre-prep security; we use the default "quite # secure" tempfile permissions instead. Magic happens later. - unless Puppet.features.microsoft_windows? + if !Puppet.features.microsoft_windows? # Grab the current file mode, and fall back to the defaults. - stat = file.lstat rescue OpenStruct.new(:mode => mode, - :uid => Process.euid, - :gid => Process.egid) - - # We only care about the bottom four slots, which make the real mode, - # and not the rest of the platform stat call fluff and stuff. - tempfile.chmod(stat.mode & 07777) - tempfile.chown(stat.uid, stat.gid) + if file.exist? + stat = file.lstat + tempfile.chown(stat.uid, stat.gid) + effective_mode = stat.mode + else + effective_mode = mode + end + + if effective_mode + # We only care about the bottom four slots, which make the real mode, + # and not the rest of the platform stat call fluff and stuff. + tempfile.chmod(effective_mode & 07777) + end end # OK, now allow the caller to write the content of the file. @@ -452,41 +460,17 @@ module Util tempfile.close if Puppet.features.microsoft_windows? - # This will appropriately clone the file, but only if the file we are - # replacing exists. Which is kind of annoying; thanks Microsoft. - # - # So, to avoid getting into an infinite loop we will retry once if the - # file doesn't exist, but only the once... - have_retried = false - - begin - # Yes, the arguments are reversed compared to the rename in the rest - # of the world. - Puppet::Util::Windows::File.replace_file(file, tempfile.path) - rescue Puppet::Util::Windows::Error - # This might race, but there are enough possible cases that there - # isn't a good, solid "better" way to do this, and the next call - # should fail in the same way anyhow. - raise if have_retried or File.exist?(file) - have_retried = true - - # OK, so, we can't replace a file that doesn't exist, so let us put - # one in place and set the permissions. Then we can retry and the - # magic makes this all work. - # - # This is the least-worst option for handling Windows, as far as we - # can determine. - File.open(file, 'a') do |fh| - # this space deliberately left empty for auto-close behaviour, - # append mode, and not actually changing any of the content. + # Windows ReplaceFile needs a file to exist, so touch handles this + if !file.exist? + FileUtils.touch(file.to_s) + if mode + Puppet::Util::Windows::Security.set_mode(mode, file.to_s) end - - # Set the permissions to what we want. - Puppet::Util::Windows::Security.set_mode(mode, file.to_s) - - # ...and finally retry the operation. - retry end + # Yes, the arguments are reversed compared to the rename in the rest + # of the world. + Puppet::Util::Windows::File.replace_file(file, tempfile.path) + else File.rename(tempfile.path, file.to_s) end --- a/spec/integration/type/file_spec.rb +++ a/spec/integration/type/file_spec.rb @@ -495,20 +495,6 @@ describe Puppet::Type.type(:file) do bucket.bucket.getfile(foomd5).should == "fooyay" bucket.bucket.getfile(barmd5).should == "baryay" end - - it "should propagate failures encountered when renaming the temporary file" do - file = described_class.new :path => path, :content => "foo" - file.stubs(:perform_backup).returns(true) - - catalog.add_resource file - - File.open(path, "w") { |f| f.print "bar" } - - File.expects(:rename).raises ArgumentError - - expect { file.write(:content) }.to raise_error(Puppet::Error, /Could not rename temporary file/) - File.read(path).should == "bar" - end end describe "when recursing" do @@ -993,6 +979,7 @@ describe Puppet::Type.type(:file) do describe "on Windows systems", :if => Puppet.features.microsoft_windows? do it "should provide valid default values when ACLs are not supported" do Puppet::Util::Windows::Security.stubs(:supports_acl?).with(source).returns false + Puppet::Util::Windows::Security.stubs(:supports_acl?).with(path).returns true file = described_class.new( :path => path, --- a/spec/unit/type/file_spec.rb +++ a/spec/unit/type/file_spec.rb @@ -1071,35 +1071,6 @@ describe Puppet::Type.type(:file) do end describe "#write" do - it "should propagate failures encountered when renaming the temporary file" do - File.stubs(:open) - File.expects(:rename).raises ArgumentError - - file[:backup] = 'puppet' - - file.stubs(:validate_checksum?).returns(false) - - property = stub('content_property', :actual_content => "something", :length => "something".length) - file.stubs(:property).with(:content).returns(property) - - expect { file.write(:content) }.to raise_error(Puppet::Error) - end - - it "should delegate writing to the content property" do - filehandle = stub_everything 'fh' - File.stubs(:open).yields(filehandle) - File.stubs(:rename) - property = stub('content_property', :actual_content => "something", :length => "something".length) - file[:backup] = 'puppet' - - file.stubs(:validate_checksum?).returns(false) - file.stubs(:property).with(:content).returns(property) - - property.expects(:write).with(filehandle) - - file.write(:content) - end - describe "when validating the checksum" do before { file.stubs(:validate_checksum?).returns(true) } --