Please review pull request #363: (#11955) Refactor usages of IO.binread opened by (joshcooper)
Description:
Monkey patch IO.binread and IO.binwrite for older rubies and refactor usages of Puppet::Util.binread.
The motivation for these methods is that, in general, we should most often be using binary mode when performing file I/O. However, text mode is the default on Windows, and using text mode can corrupt binary data, e.g. executables. So use binary mode, unless you know what text mode is and why you need to use it.
- Opened: Sat Jan 21 17:49:42 UTC 2012
- Based on: puppetlabs:2.7.x (470a6646b4faa34874ab853355730dc0471b1246)
- Requested merge: joshcooper:ticket/2.7.x/11955-binread (f28a27c56b9c1e1b4c9d6c7cb07b6462bdb2e1fa)
Diff follows:
diff --git a/lib/puppet/face/file/store.rb b/lib/puppet/face/file/store.rb
index 139181b..6b145b8 100644
--- a/lib/puppet/face/file/store.rb
+++ b/lib/puppet/face/file/store.rb
@@ -11,7 +11,7 @@
EOT
when_invoked do |path, options|
- file = Puppet::FileBucket::File.new(Puppet::Util.binread(path))
+ file = Puppet::FileBucket::File.new(IO.binread(path))
Puppet::FileBucket::File.indirection.terminus_class = :file
Puppet::FileBucket::File.indirection.save file
diff --git a/lib/puppet/file_bucket/dipper.rb b/lib/puppet/file_bucket/dipper.rb
index 27a86f8..4adab5d 100644
--- a/lib/puppet/file_bucket/dipper.rb
+++ b/lib/puppet/file_bucket/dipper.rb
@@ -31,7 +31,7 @@ def local?
# Back up a file to our bucket
def backup(file)
raise(ArgumentError, "File #{file} does not exist") unless ::File.exist?(file)
- contents = Puppet::Util.binread(file)
+ contents = IO.binread(file)
begin
file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path)
files_original_path = absolutize_path(file)
@@ -64,7 +64,7 @@ def getfile(sum)
def restore(file,sum)
restore = true
if FileTest.exists?(file)
- cursum = Digest::MD5.hexdigest(Puppet::Util.binread(file))
+ cursum = Digest::MD5.hexdigest(IO.binread(file))
# if the checksum has changed...
# this might be extra effort
diff --git a/lib/puppet/file_serving/content.rb b/lib/puppet/file_serving/content.rb
index eda141d..689e45a 100644
--- a/lib/puppet/file_serving/content.rb
+++ b/lib/puppet/file_serving/content.rb
@@ -35,7 +35,7 @@ def content
# This stat can raise an exception, too.
raise(ArgumentError, "Cannot read the contents of links unless following links") if stat.ftype == "symlink"
- @content = Puppet::Util.binread(full_path)
+ @content = IO.binread(full_path)
end
@content
end
diff --git a/lib/puppet/indirector/file_bucket_file/file.rb b/lib/puppet/indirector/file_bucket_file/file.rb
index d32788a..6f6b6ff 100644
--- a/lib/puppet/indirector/file_bucket_file/file.rb
+++ b/lib/puppet/indirector/file_bucket_file/file.rb
@@ -27,7 +27,7 @@ def find( request )
raise "could not find diff_with #{request.options[:diff_with]}" unless ::File.exists?(file2_path)
return `diff #{file_path.inspect} #{file2_path.inspect}`
else
- contents = Puppet::Util.binread(file_path)
+ contents = IO.binread(file_path)
Puppet.info "FileBucket read #{checksum}"
model.new(contents)
end
@@ -122,7 +122,7 @@ def path_for(bucket_path, digest, subfile = nil)
# If conflict_check is enabled, verify that the passed text is
# the same as the text in our file.
def verify_identical_file!(bucket_file)
- disk_contents = Puppet::Util.binread(path_for(bucket_file.bucket_path, bucket_file.checksum_data, 'contents'))
+ disk_contents = IO.binread(path_for(bucket_file.bucket_path, bucket_file.checksum_data, 'contents'))
# If the contents don't match, then we've found a conflict.
# Unlikely, but quite bad.
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 39b5964..fd34315 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -502,12 +502,6 @@ def secure_open(file,must_be_w,&block)
end
end
module_function :secure_open
-
- # Because IO#binread is only available in 1.9
- def binread(file)
- File.open(file, 'rb') { |f| f.read }
- end
- module_function :binread
end
end
diff --git a/lib/puppet/util/monkey_patches.rb b/lib/puppet/util/monkey_patches.rb
index 5186a4e..f4dd0cf 100644
--- a/lib/puppet/util/monkey_patches.rb
+++ b/lib/puppet/util/monkey_patches.rb
@@ -129,6 +129,20 @@ def lines(separator = $/)
block_given? and lines.each {|line| yield line }
lines
end
+
+ def self.binread(name, length = nil, offset = 0)
+ File.open(name, 'rb') do |f|
+ f.seek(offset) if offset > 0
+ f.read(length)
+ end
+ end unless singleton_methods.include?(:binread)
+
+ def self.binwrite(name, string, offset = 0)
+ File.open(name, 'wb') do |f|
+ f.write(offset > 0 ? string[offset..-1] : string)
+ end
+ end unless singleton_methods.include?(:binwrite)
+
end
# Ruby 1.8.5 doesn't have tap
diff --git a/spec/integration/indirector/direct_file_server_spec.rb b/spec/integration/indirector/direct_file_server_spec.rb
index 9c2e32c..231a1a5 100755
--- a/spec/integration/indirector/direct_file_server_spec.rb
+++ b/spec/integration/indirector/direct_file_server_spec.rb
@@ -22,7 +22,7 @@
it "should return an instance capable of returning its content" do
FileTest.expects(:exists?).with(@filepath).returns(true)
File.stubs(:lstat).with(@filepath).returns(stub("stat", :ftype => "file"))
- Puppet::Util.expects(:binread).with(@filepath).returns("my content")
+ IO.expects(:binread).with(@filepath).returns("my content")
instance = @terminus.find(@terminus.indirection.request(:find, "file://host#{@filepath}"))
diff --git a/spec/integration/type/file_spec.rb b/spec/integration/type/file_spec.rb
index dfdcd58..2f87918 100755
--- a/spec/integration/type/file_spec.rb
+++ b/spec/integration/type/file_spec.rb
@@ -360,7 +360,7 @@ def get_group(file)
File.open(file[:path], "wb") { |f| f.puts "bar" }
- md5 = Digest::MD5.hexdigest(Puppet::Util.binread(file[:path]))
+ md5 = Digest::MD5.hexdigest(IO.binread(file[:path]))
catalog.apply
diff --git a/spec/unit/file_bucket/dipper_spec.rb b/spec/unit/file_bucket/dipper_spec.rb
index 063aec8..a5db657 100755
--- a/spec/unit/file_bucket/dipper_spec.rb
+++ b/spec/unit/file_bucket/dipper_spec.rb
@@ -123,7 +123,7 @@ def make_tmp_file(contents)
klass.any_instance.expects(:find).with { |r| request = r }.returns(Puppet::FileBucket::File.new(contents))
dipper.restore(dest, md5).should == md5
- Digest::MD5.hexdigest(Puppet::Util.binread(dest)).should == md5
+ Digest::MD5.hexdigest(IO.binread(dest)).should == md5
request.key.should == "md5/#{md5}"
request.server.should == server
diff --git a/spec/unit/file_serving/content_spec.rb b/spec/unit/file_serving/content_spec.rb
index c1627c1..d281b79 100755
--- a/spec/unit/file_serving/content_spec.rb
+++ b/spec/unit/file_serving/content_spec.rb
@@ -102,13 +102,13 @@
it "should return the contents of the path if the file exists" do
File.expects(:stat).with(@path).returns stub("stat", :ftype => "file")
- Puppet::Util.expects(:binread).with(@path).returns(:mycontent)
+ IO.expects(:binread).with(@path).returns(:mycontent)
@content.content.should == :mycontent
end
it "should cache the returned contents" do
File.expects(:stat).with(@path).returns stub("stat", :ftype => "file")
- Puppet::Util.expects(:binread).with(@path).returns(:mycontent)
+ IO.expects(:binread).with(@path).returns(:mycontent)
@content.content
# The second run would throw a failure if the content weren't being cached.
diff --git a/spec/unit/indirector/file_bucket_file/file_spec.rb b/spec/unit/indirector/file_bucket_file/file_spec.rb
index 9141221..9f9754a 100755
--- a/spec/unit/indirector/file_bucket_file/file_spec.rb
+++ b/spec/unit/indirector/file_bucket_file/file_spec.rb
@@ -32,7 +32,7 @@ def save_bucket_file(contents, path = "/who_cares")
it "should store the path if not already stored" do
checksum = save_bucket_file("stuff\r\n", "/foo/bar")
dir_path = "#{Puppet[:bucketdir]}/f/c/7/7/7/c/0/b/fc777c0bc467e1ab98b4c6915af802ec"
- Puppet::Util.binread("#{dir_path}/contents").should == "stuff\r\n"
+ IO.binread("#{dir_path}/contents").should == "stuff\r\n"
File.read("#{dir_path}/paths").should == "foo/bar\n"
end
diff --git a/spec/unit/type/file/content_spec.rb b/spec/unit/type/file/content_spec.rb
index 108acc0..025741f 100755
--- a/spec/unit/type/file/content_spec.rb
+++ b/spec/unit/type/file/content_spec.rb
@@ -314,7 +314,7 @@
it "should copy content from the source to the file" do
@resource.write(@source)
- Puppet::Util.binread(@filename).should == @source_content
+ IO.binread(@filename).should == @source_content
end
it "should return the checksum computed" do
@@ -343,7 +343,7 @@
it "should write the contents to the file" do
@resource.write(@source)
- Puppet::Util.binread(@filename).should == @source_content
+ IO.binread(@filename).should == @source_content
end
it "should not write anything if source is not found" do
diff --git a/spec/unit/util/monkey_patches_spec.rb b/spec/unit/util/monkey_patches_spec.rb
index 4b609ad..f3a94d1 100755
--- a/spec/unit/util/monkey_patches_spec.rb
+++ b/spec/unit/util/monkey_patches_spec.rb
@@ -53,3 +53,45 @@ class Puppet::TestYamlNonInitializeClass
[1,2,3,4].combination(3).to_a.should == [[1, 2, 3], [1, 2, 4], [1, 3, 4], [2, 3, 4]]
end
end
+
+describe IO do
+ include PuppetSpec::Files
+
+ let(:file) { tmpfile('io-binary') }
+ let(:content) { "\x01\x02\x03\x04" }
+
+ describe "::binread" do
+ it "should read in binary mode" do
+ File.open(file, 'wb') {|f| f.write(content) }
+ IO.binread(file).should == content
+ end
+
+ it "should read with a length and offset" do
+ offset = 1
+ length = 2
+ File.open(file, 'wb') {|f| f.write(content) }
+ IO.binread(file, length, offset).should == content[offset..length]
+ end
+
+ it "should raise an error if the file doesn't exist" do
+ expect { IO.binread('/path/does/not/exist') }.to raise_error(Errno::ENOENT)
+ end
+ end
+
+ describe "::binwrite" do
+ it "should write in binary mode" do
+ IO.binwrite(file, content).should == content.length
+ File.open(file, 'rb') {|f| f.read.should == content }
+ end
+
+ it "should write using an offset" do
+ offset = 1
+ IO.binwrite(file, content, offset).should == content.length - offset
+ File.open(file, 'rb') {|f| f.read.should == content[offset..-1] }
+ end
+
+ it "should raise an error if the file doesn't exist" do
+ expect { IO.binwrite('/path/does/not/exist', 'foo') }.to raise_error(Errno::ENOENT)
+ end
+ end
+end
diff --git a/spec/unit/util_spec.rb b/spec/unit/util_spec.rb
index 0fc48cb..35358d6 100755
--- a/spec/unit/util_spec.rb
+++ b/spec/unit/util_spec.rb
@@ -545,19 +545,4 @@ def process_status(exitstatus)
end
end
end
-
- describe "#binread" do
- let(:contents) { "foo\r\nbar" }
-
- it "should preserve line endings" do
- path = tmpfile('util_binread')
- File.open(path, 'wb') { |f| f.print contents }
-
- Puppet::Util.binread(path).should == contents
- end
-
- it "should raise an error if the file doesn't exist" do
- expect { Puppet::Util.binread('/path/does/not/exist') }.to raise_error(Errno::ENOENT)
- end
- end
end
-- 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.
