Please review pull request #392: Ticket/2.7.x/11955 binread opened by (jeffmccune)
Description:
commit f56a09a
Author: Josh Cooper [email protected]
Date: Mon Jan 23 09:30:03 2012 -0800
(#11955) Refactor to use IO.binread
Previously, I had created a utility method `Puppet::Util.binread`,
since `IO.binread` was added in ruby 1.9. This commit monkey patches
the method, if it's not defined, to make the transition to ruby 1.9
seamless. And while I was at it, I add the corresponding `IO.binwrite`
method.
The motivation for these methods is that, in general, we should always
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.
Conflicts:
spec/unit/file_serving/content_spec.rb
commit 8202729
Author: Josh Cooper [email protected]
Date: Fri Jan 20 21:54:44 2012 -0800
(#11955) Monkey patch IO.binread and IO.binwrite
binread and binwrite are only available in ruby 1.9, so adding these
for older ruby versions.
- Opened: Mon Jan 23 17:43:19 UTC 2012
- Based on: puppetlabs:2.7.x (0a4d48fd26bf0aadb533a53fcff9d55ae9883f5f)
- Requested merge: jeffmccune:ticket/2.7.x/11955-binread (f56a09addd08550b470f4818c24fbe32b9d50b50)
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 82e3b27..5203fb0 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -480,12 +480,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 99295e1..7a9642f 100755
--- a/spec/unit/file_serving/content_spec.rb
+++ b/spec/unit/file_serving/content_spec.rb
@@ -102,15 +102,14 @@
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.
content.content
end
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.
