Subject: [sup-devel] [PATCH 2/2] {config, sources}.yaml: preserve symlinks and permissions

From: Sascha Silbe <[email-masked]>
Date: Tue, 18 Jan 2011 19:28:44 +0100

If config.yaml resp. sources.yaml already exists, preserve the permissions.
Also alter the save algorithm to overwrite the file in-place, thus leaving
symlinks intact.

Signed-off-by: Sascha Silbe <[email-masked]>
---
All invocations of save_yaml_obj now use the "backup" mode, so maybe we
should just hardcode it in save_yaml_obj, getting rid of the (existing)
safe and (new) backup parameters.

 bin/sup-config    |    2 +-
 lib/sup.rb        |   27 ++++++++++++++++++++++-----
 lib/sup/source.rb |    8 +-------
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/bin/sup-config b/bin/sup-config
index 132fd77..c4a64a3 100755
--- a/bin/sup-config
+++ b/bin/sup-config
@@ -191,7 +191,7 @@ else
   end
 end
 
-Redwood::save_yaml_obj $config, Redwood::CONFIG_FN
+Redwood::save_yaml_obj $config, Redwood::CONFIG_FN, false, true
 
 say "Ok, I've saved you up a nice lil' #{Redwood::CONFIG_FN}."
 
diff --git a/lib/sup.rb b/lib/sup.rb
index 09744b4..21bddb2 100644
--- a/lib/sup.rb
+++ b/lib/sup.rb
@@ -86,7 +86,7 @@ module Redwood
   module_function :reporting_thread, :record_exception, :exceptions
 
 ## one-stop shop for yamliciousness
-  def save_yaml_obj o, fn, safe=false
+  def save_yaml_obj o, fn, safe=false, backup=false
     o = if o.is_a?(Array)
       o.map { |x| (x.respond_to?(:before_marshal) && x.before_marshal) || x }
     elsif o.respond_to? :before_marshal
@@ -95,16 +95,33 @@ module Redwood
       o
     end
 
-    if safe
+    mode = if File.exists? fn
+      File.stat(fn).mode
+    else
+      0600
+    end
+
+    if backup
+      backup_fn = fn + '.bak'
+      unless File.exists?(backup_fn) && File.size(fn) == 0
+        File.open(backup_fn, "w", mode) do |f|
+          File.open(fn, "r") { |old_f| FileUtils.copy_stream old_f, f }
+          f.fsync
+        end
+      end
+      File.open(fn, "w") do |f|
+        f.puts o.to_yaml
+        f.fsync
+      end
+    elsif safe
       safe_fn = "#{File.dirname fn}/safe_#{File.basename fn}"
-      mode = File.stat(fn).mode if File.exists? fn
       File.open(safe_fn, "w", mode) do |f|
         f.puts o.to_yaml
         f.fsync
       end
       FileUtils.mv safe_fn, fn
     else
-      File.open(fn, "w") do |f|
+      File.open(fn, "w", mode) do |f|
         f.puts o.to_yaml
         f.fsync
       end
@@ -292,7 +309,7 @@ EOS
         :col_jump => 2
       }
       begin
-        Redwood::save_yaml_obj config, filename
+        Redwood::save_yaml_obj config, filename, false, true
       rescue StandardError => e
         $stderr.puts "warning: #{e.message}"
       end
diff --git a/lib/sup/source.rb b/lib/sup/source.rb
index 9c398f7..f2379fb 100644
--- a/lib/sup/source.rb
+++ b/lib/sup/source.rb
@@ -212,13 +212,7 @@ class SourceManager
   def save_sources fn=Redwood::SOURCE_FN
     @source_mutex.synchronize do
       if @sources_dirty
-        bakfn = fn + ".bak"
-        if File.exists? fn
-          File.chmod 0600, fn
-          FileUtils.mv fn, bakfn, :force => true unless File.exists?(bakfn) && File.size(fn) == 0
-        end
-        Redwood::save_yaml_obj sources, fn, true
-        File.chmod 0600, fn
+        Redwood::save_yaml_obj sources, fn, false, true
       end
       @sources_dirty = false
     end
-- 
1.7.2.3

_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: William Morgan <[email-masked]>
Date: Wed, 19 Jan 2011 12:17:23 -0500

Reformatted excerpts from Sascha Silbe's message of 2011-01-19:
> Shall I send a fix-up patch?

I can descend from the heavens and apply my fix to master directly,
unless Rich objects.
-- 
William <[email-masked]>
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: William Morgan <[email-masked]>
Date: Wed, 19 Jan 2011 12:19:11 -0500

Reformatted excerpts from William Morgan's message of 2011-01-19:
> Reformatted excerpts from Sascha Silbe's message of 2011-01-18:
> This line is giving me some problems when fn doesn't exist (e.g. when
> running sup-sync the first time without a config.yaml). Should it be
> 
>   File.exists?(backup_fn) && File.exists?(fn) && File.size(fn) == 0
> 
> instead?

Actually, I think it should be:

  if File.exists?(fn) && File.size(fn) > 0

Which has the advantage of actually working...
-- 
William <[email-masked]>
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Ben Walton <[email-masked]>
Date: Wed, 19 Jan 2011 13:41:21 -0500

>   if File.exists?(fn) && File.size(fn) > 0

How about:

begin
   fninf = File.stat(fn)
   if fninf.size? ...

rescue Errno::ENOENT => e
...
end
       
I didn't look at the surrounding code so I'm not sure how to best
integrate this.  The advantage is that you save one user->kernel space
traversal.  Calling .exists? and .size via File results in two stat()
calls.  Grabbing and saving the actual stat result on the first call
and then using them for further tests saves this.  Not a huge
overhead, I admit...

HTH.
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

---

From: Rich Lane <[email-masked]>
Date: Tue, 18 Jan 2011 22:41:53 -0500

Both patches applied to master.
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Sascha Silbe <[email-masked]>
Date: Tue, 18 Jan 2011 19:28:43 +0100

Before renaming sources.yaml we need to fsync() it, otherwise we could end up
with an empty file in case of a crash [1].

[1] http://thunk.org/tytso/blog/2009/03/12/delayed-allocation-and-the-zero-length-file-problem/

Signed-off-by: Sascha Silbe <[email-masked]>
---
 lib/sup.rb |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/sup.rb b/lib/sup.rb
index edd23c8..09744b4 100644
--- a/lib/sup.rb
+++ b/lib/sup.rb
@@ -98,10 +98,16 @@ module Redwood
     if safe
       safe_fn = "#{File.dirname fn}/safe_#{File.basename fn}"
       mode = File.stat(fn).mode if File.exists? fn
-      File.open(safe_fn, "w", mode) { |f| f.puts o.to_yaml }
+      File.open(safe_fn, "w", mode) do |f|
+        f.puts o.to_yaml
+        f.fsync
+      end
       FileUtils.mv safe_fn, fn
     else
-      File.open(fn, "w") { |f| f.puts o.to_yaml }
+      File.open(fn, "w") do |f|
+        f.puts o.to_yaml
+        f.fsync
+      end
     end
   end
 
-- 
1.7.2.3

---

From: Rich Lane <[email-masked]>
Date: Wed, 19 Jan 2011 13:44:07 -0500

> I can descend from the heavens and apply my fix to master directly,

Please do.

---

From: Sascha Silbe <[email-masked]>
Date: Wed, 19 Jan 2011 17:42:28 +0100

>   File.exists?(backup_fn) && File.exists?(fn) && File.size(fn) == 0

Yes, you're right. We shouldn't try to do a copy if the original doesn't
exist. Sorry for not testing that case.
Shall I send a fix-up patch?

Sascha

-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Sascha Silbe <[email-masked]>
Date: Wed, 19 Jan 2011 11:05:06 +0100

> Both patches applied to master.
[as well as a whole bunch of other patches from me]

Thanks!

Sascha

---

From: William Morgan <[email-masked]>
Date: Wed, 19 Jan 2011 10:38:11 -0500

instead?