On 03/04/10 18:54, Markus Roberts wrote:
> 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.
You're right. I'm so used to never read comments, that I don't see them
anymore.
>> 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) } }
>
I don't know. Does withumask return what the block returns (and same
question for File.open)?
That's the kind of things I'm never sure of, so I tend to do defensive
code :-)
>> # 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 ""
I mimicked what ensure was doing when there was no source nor content.
> 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 might be right because the file is open with trunc and create.
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--
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.
