[Pkg-puppet-devel] [SCM] Puppet packaging for Debian branch, upstream, updated. puppet-0.24.5-rc3-1601-gf8c1b08

[James Turnbull]

The following commit has been merged in the upstream branch:
commit e9a0cb7a28a34fd04db4bfe1db347da5d774f2e8
Author: Markus Roberts <Markus at reality.com>
Date:   Fri Dec 18 13:40:29 2009 -0800

    Fix for #2657 (retain old setting if config has syntax error)
    
    This appears to be regression introduced by threading changes.  The fix was
    to rearrange things to keep the old behaviour (don't clear the settings
    until you know the config file parses) and the new (don't nest calls to
    synchronize) by:
    
    1. Splitting clear into two parts--clear, which works as before, and
    unsafe_clear which it calls and which expects synchronization to be
    handled externally.
    
    2. Rearranging the code to recover the previous calling order
    
    3. Trapping syntax errors and turning them into logged messages and a
    no-op effect.
    
    4. Fixing reparse to not wrap a call to this code with a synchronize.
    
    5. Tests.
    
    Signed-off-by: Markus Roberts <Markus at reality.com>

diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index e80c7cc..df07d5c 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -64,20 +64,25 @@ class Puppet::Util::Settings
     # Remove all set values, potentially skipping cli values.
     def clear(exceptcli = false)
         @sync.synchronize do
-            @values.each do |name, values|
-                @values.delete(name) unless exceptcli and name == :cli
-            end
+            unsafe_clear(exceptcli)
+        end
+    end
+    
+    # Remove all set values, potentially skipping cli values.
+    def unsafe_clear(exceptcli = false)
+        @values.each do |name, values|
+            @values.delete(name) unless exceptcli and name == :cli
+        end
 
-            # Don't clear the 'used' in this case, since it's a config file reparse,
-            # and we want to retain this info.
-            unless exceptcli
-                @used = []
-            end
+        # Don't clear the 'used' in this case, since it's a config file reparse,
+        # and we want to retain this info.
+        unless exceptcli
+            @used = []
+        end
 
-            @cache.clear
+        @cache.clear
 
-            @name = nil
-        end
+        @name = nil
     end
 
     # This is mostly just used for testing.
@@ -317,23 +322,25 @@ class Puppet::Util::Settings
         # and reparsed if necessary.
         set_filetimeout_timer()
 
-        # Retrieve the value now, so that we don't lose it in the 'clear' call.
-        file = self[:config]
-
-        return unless FileTest.exist?(file)
-
-        # We have to clear outside of the sync, because it's
-        # also using synchronize().
-        clear(true)
-
         @sync.synchronize do
-            unsafe_parse(file)
+            unsafe_parse(self[:config])
         end
     end
 
     # Unsafely parse the file -- this isn't thread-safe and causes plenty of problems if used directly.
     def unsafe_parse(file)
-        parse_file(file).each do |area, values|
+        return unless FileTest.exist?(file)
+        begin
+            data = parse_file(file)
+        rescue => details
+            puts details.backtrace if Puppet[:trace]
+            Puppet.err "Could not parse #{file}: #{details}"
+            return
+        end
+
+        unsafe_clear(true)
+
+        data.each do |area, values|
             @values[area] = values
         end
 
@@ -425,9 +432,7 @@ class Puppet::Util::Settings
     def reparse
         if file and file.changed?
             Puppet.notice "Reparsing %s" % file.file
-            @sync.synchronize do
-                parse
-            end
+            parse
             reuse()
         end
     end
diff --git a/spec/unit/util/settings.rb b/spec/unit/util/settings.rb
index aa2101f..5642813 100755
--- a/spec/unit/util/settings.rb
+++ b/spec/unit/util/settings.rb
@@ -579,6 +579,25 @@ describe Puppet::Util::Settings do
             # and we should now have the new value in memory
             @settings[:two].should == "disk-replace"
         end
+
+        it "should retain in-memory values if the file has a syntax error" do
+            # Init the value
+            text = "[main]\none = initial-value\n"
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
+            @settings[:one].should == "initial-value"
+
+            # Now replace the value with something bogus
+            text = "[main]\nkenny = killed-by-what-follows\n1 is 2, blah blah florp\n"
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
+
+            # The originally-overridden value should not be replaced with the default
+            @settings[:one].should == "initial-value"
+
+            # and we should not have the new value in memory
+            @settings[:kenny].should be_nil
+        end
     end
 
     it "should provide a method for creating a catalog of resources from its configuration" do

-- 
Puppet packaging for Debian