Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

closes #2: atomic file save with backup #6

Open
wants to merge 1 commit into from

2 participants

@steakknife

Implemented per request using rename.

As implemented, it also create netrc.### backup files before saving in addition to the tempfile.

Behavior easily disabled by passing , false to atomic_write.

@steakknife
Run options: 

# Running tests:

....................................

Finished tests in 0.271142s, 132.7718 tests/s, 199.1576 assertions/s.

36 tests, 54 assertions, 0 failures, 0 errors, 0 skips
lib/file_utility.rb
@@ -0,0 +1,148 @@
+require 'tempfile'
+
+module FileUtility
@geemus Owner
geemus added a note

This should probably be namespaced inside Netrc to avoid collisions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/file_utility.rb
((56 lines not shown))
+ file_handles = file_names.collect { |filename| File.open(filename, 'rb:ASCII-8BIT') }
+ # Compare files
+ return (block_given?) ?
+ synchronous_compare_file_handles(file_handles, size, &block) :
+ synchronous_compare_file_handles(file_handles, size) { |x,y| x == y }
+ ensure
+ file_handles.map { |f| f.close rescue nil }
+ end
+ end
+
+ def self.copy_file(source, dest, mode = 0600, verify = true, block_size = BUFFER_SIZE, &block)
+ File.open(source, 'rb') do |s|
+ File.open(dest, 'wb', mode) do |d|
+ begin
+ begin # Zero copy version
+ require 'io/splice' # gem install io_splice # linux only
@geemus Owner
geemus added a note

I don't know if adding the splice requirement is desirable here.

Yeap, that was for a different use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/file_utility.rb
((105 lines not shown))
+ # Make sure the file is readable before creating a backup file
+ File.open(filename, 'rb') {}
+ n = -1
+ begin
+ backup_filename = backup_filename_pattern % [ filename, n += 1 ]
+ end while File.exist?(backup_filename)
+
+ copy_file filename, backup_filename, File.stat(filename).mode
+ end
+
+ # Rename files atomicly, backup by default.
+ #
+ # Pass mode to ensure dest is set to a particular file mode, otherwise it is copied from source.
+ #
+ def self.safe_rename(source, dest, make_backup = true, mode = nil)
+ backup(dest) if make_backup
@geemus Owner
geemus added a note

Backup might not really be needed. I think, if nothing else, that the added complexity required for it seems like more trouble than perhaps it is really worth.

Again, different use case. Don't want to fill up dir with a bunch of backups obviously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/file_utility.rb
((108 lines not shown))
+ begin
+ backup_filename = backup_filename_pattern % [ filename, n += 1 ]
+ end while File.exist?(backup_filename)
+
+ copy_file filename, backup_filename, File.stat(filename).mode
+ end
+
+ # Rename files atomicly, backup by default.
+ #
+ # Pass mode to ensure dest is set to a particular file mode, otherwise it is copied from source.
+ #
+ def self.safe_rename(source, dest, make_backup = true, mode = nil)
+ backup(dest) if make_backup
+ mode ||= File.stat(source).mode
+ File.rename source, dest
+ File.chmod(mode, dest) if mode != File.stat(dest).mode
@geemus Owner
geemus added a note

chmod should be skipped on windows, as it doesn't really do anything in most cases and will raise errors for non-ascii characters in paths.

The reason is that renaming temp files leaving yucky setuid. Granted, the mktemp call should be making a file right there to prevent any cross-filesystem hierarchy weirdness.

chown medical experimentation on various platforms:

Lucid works fine:

Using /home/ballard/.rvm/gems/ree-1.8.7-2012.01
~  ᐅ ( umask 777 ; touch ✪ ; ruby -rfileutils -e "FileUtils.chmod(0444,'✪')" ; ls -l ✪ ; rm -f ✪ )
-r--r--r-- 1 ballard ballard 0 2012-03-13 19:14 ✪
~  ᐅ rvm use 1.9.3
Using /home/ballard/.rvm/gems/ruby-1.9.3-p125
~  ᐅ ( umask 777 ; touch ✪ ; ruby -rfileutils -e "FileUtils.chmod(0444,'✪')" ; ls -l ✪ ; rm -f ✪ )
-r--r--r-- 1 ballard ballard 0 2012-03-13 19:14 ✪

Lion works fine:

~  ᐅ rvm use ree
Using /Volumes/Users/barry/.rvm/gems/ree-1.8.7-2012.02
~  ᐅ ( umask 777 ; touch ✪ ; ruby -rfileutils -e "FileUtils.chmod(0444,'✪')" ; ls -l ✪ ; rm -f ✪ )
-r--r--r--  1 barry  admin  0 Mar 13 12:15 ✪
~  ᐅ rvm use 1.9.3
Using /Volumes/Users/barry/.rvm/gems/ruby-1.9.3-p125
~  ᐅ ( umask 777 ; touch ✪ ; ruby -rfileutils -e "FileUtils.chmod(0444,'✪')" ; ls -l ✪ ; rm -f ✪ )
-r--r--r--  1 barry  admin  0 Mar 13 12:15 ✪
~  ᐅ rvm use jruby
Using /Volumes/Users/barry/.rvm/gems/jruby-1.6.7
~  ᐅ ( umask 777 ; touch ✪ ; ruby -rfileutils -e "FileUtils.chmod(0444,'✪')" ; ls -l ✪ ; rm -f ✪ )
-r--r--r--  1 barry  admin  0 Mar 13 12:15 ✪
~  ᐅ rvm use rbx
zsh: correct 'rbx' to '.rbx' [nyae]? n
Using /Volumes/Users/barry/.rvm/gems/rbx-head
~  ᐅ ( umask 777 ; touch ✪ ; ruby -rfileutils -e "FileUtils.chmod(0444,'✪')" ; ls -l ✪ ; rm -f ✪ )
-r--r--r--  1 barry  admin  0 Mar 13 12:15 ✪
~  ᐅ rvm use system
Now using system ruby.
~  ᐅ ( umask 777 ; touch ✪ ; ruby -rfileutils -e "FileUtils.chmod(0444,'✪')" ; ls -l ✪ ; rm -f ✪ )
-r--r--r--  1 barry  admin  0 Mar 13 12:16 ✪

Windows (tested on Windows 2003):

Cygwin: Was able to chmod and create a unicode character filename under cygwin. Windows could not display it, but it appeared correctly over a share. chmoding change NTFS ACEs mapping to (current user, Everyone and pseudo-real user None) per whatever mode is passed.

RubyInstaller 1.9.3: chown throws Error::EINVAL, even running via a unicode script with the magic header.

Cygwin looks like the best way to support unicode Ruby on Windows.

@geemus Owner
geemus added a note

My experience on windows is similar to the rubyinstaller one you mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@geemus
Owner

@steakknife added some inline comments. Also, could you squash the commits? Thanks!

@geemus
Owner

@dpiddy Mind taking a look at this as well? I think you had a better sense of what you wanted here than I likely do.

@steakknife

SCISPOPD.

lib/netrc.rb
((6 lines not shown))
[new_item_prefix+"machine ", m, "\n login ", l, "\n password ", p, "\n"]
end
def save
- File.open(@path, 'w', 0600) {|file| file.print(unparse)}
+ Netrc.parse(Netrc.lex((unparsed = unparse).split "\n"))
+ FileUtility.atomic_write(@path) {|file| file.print(unparsed) }
@geemus Owner
geemus added a note

Should probably still namespace this as Netrc::FileUtility, just in case.

Okie dokie.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@geemus
Owner

Looks like the new commit is lacking the lib/file_utility.rb file? Could you double check that? Thanks!

@steakknife

Yup. Was in the middle of fixing that. ;)

Barry Allard closes #2 cc31e92
@geemus
Owner

@steakknife It doesn't look to me like copy_file, synchronous_compare_file and synchronous_compare_file_handle are ever actually called. Is that the case? Can we remove them? Sorry for all the run around, turns out atomic stuff is complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 13, 2012
  1. closes #2

    Barry Allard authored
This page is out of date. Refresh to see the latest.
Showing with 150 additions and 1 deletion.
  1. +129 −0 lib/file_utility.rb
  2. +9 −1 lib/netrc.rb
  3. +12 −0 test/test_netrc.rb
View
129 lib/file_utility.rb
@@ -0,0 +1,129 @@
+
+class Netrc
+ module FileUtility
+ BUFFER_SIZE=256*1024
+
+ # Compare data of 2+ file handles using block
+ def self.synchronous_compare_file_handles(file_handles, bytes, buffer_size = BUFFER_SIZE, &block)
+ return true if bytes == 0
+
+ reference_file = file_handles[0]
+ other_files = file_handles[1..-1]
+
+ reference_buffer = (0.chr) * buffer_size
+ other_buffer = reference_buffer.clone
+
+ # Until there is no more data
+ begin
+ bytes_read = reference_file.read(buffer_size, reference_buffer).bytes.count
+
+ # Read a block and compare it from other files
+ other_files.each do |f|
+ # Read other data
+ other_bytes_read = f.read(buffer_size, other_buffer).bytes.count
+ raise 'Synchronous_compare cannot handle io read()s of different sizes' if bytes_read != other_bytes_read
+ # FAIL: other data and reference data differ
+ return false unless yield(reference_buffer, other_buffer)
+ end
+ end until (bytes -= bytes_read) == 0
+
+ # SUCCESS: no more bytes and all blocks same
+ return true
+ end
+
+ # Pass a block to compare blocks other than using == operator.
+ #
+ # buffer_size defaults to 256k per file
+ #
+ # Returns true if all files are equal
+ # Returns false if any files differ in content or size
+ #
+ def self.synchronous_compare_files(*file_names, &block)
+ raise ArgumentError, 'Requires at least 2 file names' unless file_names.size >= 2
+ # Check that all files are the same size
+ size = nil
+ file_names.each do |f|
+ current_size = File.size(f)
+ size ||= current_size
+ # File sizes differ, so the files differ
+ return false unless current_size == size
+ end
+ return true if size == 0 # Don't bother opening zero byte files
+
+ file_handles = []
+ begin # close all files if ever leaving this block
+ # Open all files
+ file_handles = file_names.collect { |filename| File.open(filename, 'rb:ASCII-8BIT') }
+ # Compare files
+ return (block_given?) ?
+ synchronous_compare_file_handles(file_handles, size, &block) :
+ synchronous_compare_file_handles(file_handles, size) { |x,y| x == y }
+ ensure
+ file_handles.map { |f| f.close rescue nil }
+ end
+ end
+
+ def self.copy_file(source, dest, mode = 0600, verify = true, block_size = BUFFER_SIZE, &block)
+ File.open(source, 'rb') do |s|
+ File.open(dest, 'wb', mode) do |d|
+ begin
+ buffer = 0.chr * block_size
+ until s.eof?
+ s.read(block_size, buffer)
+ d.syswrite(buffer)
+ end
+ rescue Exception
+ d.close rescue nil
+ d.unlink rescue nil
+ raise
+ end
+ end # d
+ end # s
+ IO.fsync rescue NotImplementedError
+ if verify
+ raise 'File differs from original' unless synchronous_compare_files source, dest, &block
+ end
+ end
+
+ # Proper Windows detection.
+ #
+ def self.windows?
+ /win(32|dows|ce)|djgpp|(ms|cyg|bcc)win|mingw32/i || ENV['OS'] == 'Windows_NT'
+ end
+
+ # Rename files atomicly.
+ #
+ # Pass mode to ensure dest is set to a particular file mode, otherwise it is copied from source.
+ #
+ def self.safe_rename(source, dest, mode = nil)
+ mode ||= File.stat(source).mode unless windows?
+ result = File.rename source, dest
+ (File.chmod(mode, dest) if !windows? and mode != File.stat(dest).mode) rescue Errno::EINVAL
+ result
+ end
+
+ # Safely write to a file that may exist.
+ #
+ def self.atomic_write(filename, mode = 0600, &block)
+ raise ArgumentError, 'Must specify a block' unless block_given?
+
+ if File.exist?(filename)
+ File.open(filename, 'ab+') { } # Make sure the file exists and is read-writable
+ src_mode = File.stat(filename).mode unless windows?
+ require 'tempfile'
+ Tempfile.open(File.basename(filename), File.dirname(filename)) do |tempfile|
+ begin
+ yield(tempfile)
+ tempfile.close
+ safe_rename tempfile.path, filename, src_mode
+ ensure
+ tempfile.close rescue nil
+ tempfile.unlink rescue nil
+ end
+ end
+ else # destination file does not exist
+ File.open(filename, 'wb+', mode, &block)
+ end
+ end
+ end # module FileUtility
+end # class Netrc
View
10 lib/netrc.rb
@@ -1,4 +1,6 @@
+
class Netrc
+
VERSION = "0.7"
WINDOWS = (RUBY_PLATFORM =~ /win32|mingw32/i)
@@ -113,6 +115,8 @@ def [](k)
end
def []=(k, info)
+ raise Error, "Login cannot be blank" unless info[0].size > 0
+ raise Error, "Password cannot be blank" unless info[1].size > 0
if item = @data.detect {|datum| datum[1] == k}
item[3], item[5] = info
else
@@ -140,11 +144,15 @@ def each(&block)
end
def new_item(m, l, p)
+ raise Error, "Login cannot be blank" unless l.size > 0
+ raise Error, "Password cannot be blank" unless p.size > 0
[new_item_prefix+"machine ", m, "\n login ", l, "\n password ", p, "\n"]
end
def save
- File.open(@path, 'w', 0600) {|file| file.print(unparse)}
+ Netrc.parse(Netrc.lex((unparsed = unparse).split "\n"))
+ require File.join(File.dirname(__FILE__) , 'file_utility')
+ Netrc::FileUtility.atomic_write(@path) {|file| file.print(unparsed) }
end
def unparse
View
12 test/test_netrc.rb
@@ -101,4 +101,16 @@ def test_save_create
n.save
assert_equal(0600, File.stat("/tmp/created.netrc").mode & 0777)
end
+
+ def test_dont_save_bad
+ FileUtils.rm_f("/tmp/foo")
+ netrc = Netrc.read("/tmp/foo")
+ assert_raise(Netrc::Error) do
+ netrc["test.local"] = ["", "hello"]
+ end
+ netrc.save
+ assert_nothing_raised do
+ Netrc.read("/tmp/foo")
+ end
+ end
end
Something went wrong with that request. Please try again.