Initial thoughts (I'll be back for more later):
Looks Ok at a first read, though I have question/comments (some noted below)
and parts I'll need to go back to with more coffee in me to make sure I
understand.
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
> index e13c796..93d1735 100644
> --- a/lib/puppet/type/file.rb
> +++ b/lib/puppet/type/file.rb
> @@ -706,10 +706,10 @@ module Puppet
>
> # Write out the file. Requires the content to be written,
> # the property name for logging, and the checksum for validation.
> - def write(content, property)
> + def write(property)
>
It appears that the comment here is now at least two refactors out of date,
as we are nolonger passing in the content or the checksum. This might be a
good argument for removing the comment altogether as it is both a) wrong and
b) being consistently ignored.
> remove_existing(:file)
>
> - use_temporary_file = (content.length != 0)
> + use_temporary_file = write_temporary_file?
> if use_temporary_file
> path = "#{self[:path]}.puppettmp_#{rand(10000)}"
> while File.exists?(path) or File.symlink?(path)
> @@ -722,14 +722,17 @@ module Puppet
> mode = self.should(:mode) # might be nil
> umask = mode ? 000 : 022
>
> + content_checksum = nil
> Puppet::Util.withumask(umask) do
> - File.open(path, File::CREAT|File::WRONLY|File::TRUNC,
> mode) { |f| f.print content }
> + File.open(path, File::CREAT|File::WRONLY|File::TRUNC,
> mode) do |f|
> + content_checksum = write_content(f)
> + end
> end
>
>
This could be reduced to:
content_checksum = Puppet::Util.withumask(umask) do
File.open(path, File::CREAT|File::WRONLY|File::TRUNC, mode)
{ |f| write_content(f) }
end
Or if I'm not mistaken:
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.
> begin
> - fail_if_checksum_is_wrong(path, content)
> + fail_if_checksum_is_wrong(path, content_checksum) if
> validate_checksum?
> File.rename(path, self[:path])
> rescue => detail
> fail "Could not rename temporary file %s to %s: %s" %
> [path, self[:path], detail]
> @@ -743,17 +746,35 @@ module Puppet
> property_fix
> end
>
> - private
> + # Should we validate the checksum of the file we're writing?
> + def validate_checksum?
> + self[:checksum] !~ /time/
> + end
>
> # Make sure the file we wrote out is what we think it is.
> - def fail_if_checksum_is_wrong(path, content)
> - # Use the appropriate checksum type -- md5, md5lite, etc.
> - checksum = parameter(:checksum).sum(content)
> -
> + def fail_if_checksum_is_wrong(path, content_checksum)
> newsum = parameter(:checksum).sum_file(path)
> - return if [:absent, nil, checksum].include?(newsum)
> + return if [:absent, nil, content_checksum].include?(newsum)
> +
> + self.fail "File written to disk did not match checksum;
> discarding changes (%s vs %s)" % [content_checksum, newsum]
> + end
>
> - self.fail "File written to disk did not match checksum;
> discarding changes (#{checksum} vs #{newsum})"
> + def write_content(file)
> + checksum = nil
> + if content = property(:content)
> + checksum = content.write(file)
> + else # we have a sole ensure property
> + file.print ""
> + end
> + checksum
> + end
>
> What is the purpose of the
file.print ""
That is, opening a file and then closing it without writing anything still
creates it in all OSes, right? Isn't this routine doing the same thing as:
def write_content(file)
(content = property(:content)) && content.write(file)l
end
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-...@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
