Protecting from bug in Syslog, and fixing some more log messages

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@734 980ebf18-57e1-0310-9a29-db15c13687c0

7 files changed, 79 insertions, 66 deletions

diff --git a/lib/puppet/element.rb b/lib/puppet/element.rb
index a5cd59fb5..26c82ddf5 100644
--- a/lib/puppet/element.rb
+++ b/lib/puppet/element.rb
@@ -27,7 +27,8 @@ class Puppet::Element
     # so raise an error if a method that isn't overridden gets called
     @@interface_methods.each { |method|
         self.send(:define_method,method) {
-            raise "%s has not overridden %s" % [self.class,method]
+            raise Puppet::DevError, "%s has not overridden %s" %
+                [self.class,method]
         }
     }
     #---------------------------------------------------------------
diff --git a/lib/puppet/log.rb b/lib/puppet/log.rb
index 1c9a5bd7a..62fe98fdf 100644
--- a/lib/puppet/log.rb
+++ b/lib/puppet/log.rb
@@ -163,14 +163,19 @@ module Puppet # :nodoc:
                 case dest
                 when Module # This is the Syslog module
                     next if msg.remote
+                    # XXX Syslog currently has a bug that makes it so you
+                    # cannot log a message with a '%' in it.  So, we get rid
+                    # of them.
                     if msg.source == "Puppet"
-                        dest.send(msg.level,msg.to_s)
+                        dest.send(msg.level, msg.to_s.gsub("%", ''))
                     else
-                        dest.send(msg.level,"(%s) %s" % [msg.source,msg.to_s])
+                        dest.send(msg.level, "(%s) %s" %
+                            [msg.source.to_s.gsub("%", ""), msg.to_s.gsub("%", '')]
+                        )
                     end
                 when File:
                     dest.puts("%s %s (%s): %s" %
-                        [msg.time,msg.source,msg.level,msg.to_s])
+                        [msg.time, msg.source, msg.level, msg.to_s])
                 when :console
                     if msg.source == "Puppet"
                         puts @colors[msg.level] + "%s: %s" % [
@@ -202,8 +207,8 @@ module Puppet # :nodoc:
                         end
                     end
                 else
-                    #raise Puppet::Error, "Invalid log destination %s" % dest
-                    puts "Invalid log destination %s" % dest.inspect
+                    raise Puppet::Error, "Invalid log destination %s" % dest
+                    #puts "Invalid log destination %s" % dest.inspect
                 end
             }
         end
@@ -270,8 +275,8 @@ module Puppet # :nodoc:
                     @source = args[:source].to_s
                 end
                 unless defined? @tags and @tags
-                    if @source.respond_to?(:tags)
-                        @tags = @source.tags
+                    if args[:source].respond_to?(:tags)
+                        @tags = args[:source].tags
                     end
                 end
             else
diff --git a/lib/puppet/storage.rb b/lib/puppet/storage.rb
index 24a6b6920..dfcb5fe55 100644
--- a/lib/puppet/storage.rb
+++ b/lib/puppet/storage.rb
@@ -39,7 +39,7 @@ module Puppet
                     begin
                         @@state[eval(myclass)][key] = Marshal::load(value)
                     rescue => detail
-                        raise RuntimeError,
+                        raise Puppet::Error,
                             "Failed to load value for %s::%s => %s" % [
                                 myclass,key,detail
                             ], caller
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index ca7a3cfd9..8a08152f8 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1,8 +1,3 @@
-# This class is the abstract base class for the mechanism for organizing
-# work.  No work is actually done by this class or its subclasses; rather,
-# the subclasses include states which do the actual work.
-#   See state.rb for how work is actually done.
-
 require 'puppet'
 require 'puppet/log'
 require 'puppet/element'
@@ -11,7 +6,11 @@ require 'puppet/metric'
 require 'puppet/type/state'
 # see the bottom of the file for the rest of the inclusions
 
-module Puppet
+module Puppet # :nodoc:
+# This class is the abstract base class for the mechanism for organizing
+# work.  No work is actually done by this class or its subclasses; rather,
+# the subclasses include states which do the actual work.
+#   See state.rb for how work is actually done.
 class Type < Puppet::Element
     attr_accessor :children, :parameters, :parent
     attr_accessor :file, :line, :tags
@@ -375,8 +374,9 @@ class Type < Puppet::Element
 
     # abstract accessing parameters and states, and normalize
     # access to always be symbols, not strings
-    # XXX this returns a _value_, not an object
-    # if you want a state object, use <type>.state(<state>)
+    # This returns a value, not an object.  It returns the 'is'
+    # value, but you can also specifically return 'is' and 'should'
+    # values using 'object.is(:state)' or 'object.should(:state)'.
     def [](name)
         if name.is_a?(String)
             name = name.intern
@@ -402,8 +402,9 @@ class Type < Puppet::Element
         end
     end
 
-    # abstract setting parameters and states, and normalize
-    # access to always be symbols, not strings
+    # Abstract setting parameters and states, and normalize
+    # access to always be symbols, not strings.  This sets the 'should'
+    # value on states, and otherwise just sets the appropriate parameter.
     def []=(name,value)
         if name.is_a?(String)
             name = name.intern
@@ -427,7 +428,12 @@ class Type < Puppet::Element
                 if @states.include?(name)
                     @states[name].should = value
                 else
-                    newstate(name, :should => value)
+                    # newstate returns true if it successfully created the state,
+                    # false otherwise; I just don't know what to do with that
+                    # fact.
+                    unless newstate(name, :should => value)
+                        #self.info "%s failed" % name
+                    end
                 end
             end
         elsif self.class.validparameter?(name)
@@ -533,35 +539,38 @@ class Type < Puppet::Element
 
     # create a new state
     def newstate(name, hash = {})
-        if stateklass = self.class.validstate?(name) 
-            if @states.include?(name)
-                hash.each { |var,value|
-                    @states[name].send(var.to_s + "=", value)
-                }
-            else
-                #Puppet.warning "Creating state %s for %s" %
-                #    [stateklass.name,self.name]
+        unless stateklass = self.class.validstate?(name) 
+            raise Puppet::Error, "Invalid parameter %s" % name
+        end
+        if @states.include?(name)
+            hash.each { |var,value|
+                @states[name].send(var.to_s + "=", value)
+            }
+        else
+            #Puppet.warning "Creating state %s for %s" %
+            #    [stateklass.name,self.name]
+            begin
                 hash[:parent] = self
-                begin
-                    # make sure the state doesn't have any errors
-                    newstate = stateklass.new(hash)
-                    @states[name] = newstate
-                rescue Puppet::Error => detail
-                    # the state failed, so just ignore it
-                    self.warning "State %s failed: %s" %
-                        [name, detail]
-                rescue Puppet::DevError => detail
-                    # the state failed, so just ignore it
-                    self.err "State %s failed: %s" %
-                        [name, detail]
-                rescue => detail
-                    # the state failed, so just ignore it
-                    self.err "State %s failed: %s (%s)" %
-                        [name, detail, detail.class]
-                end
+                # make sure the state doesn't have any errors
+                newstate = stateklass.new(hash)
+                @states[name] = newstate
+                return true
+            rescue Puppet::Error => detail
+                # the state failed, so just ignore it
+                self.warning "State %s failed: %s" %
+                    [name, detail]
+                return false
+            rescue Puppet::DevError => detail
+                # the state failed, so just ignore it
+                self.err "State %s failed: %s" %
+                    [name, detail]
+                return false
+            rescue => detail
+                # the state failed, so just ignore it
+                self.err "State %s failed: %s (%s)" %
+                    [name, detail, detail.class]
+                return false
             end
-        else
-            raise Puppet::Error, "Invalid parameter %s" % name
         end
     end
 
@@ -778,15 +787,10 @@ class Type < Puppet::Element
         order.flatten.each { |name|
             if hash.include?(name)
                 begin
-                    if name == "owner" or name == :owner
-                        if hash[name].nil?
-                            puts caller
-                        end
-                    end
                     self[name] = hash[name]
                 rescue => detail
                     raise Puppet::DevError.new( 
-                        "Could not set %s on %s: %s" % [name, self.class, detail]
+                        "Could not set %s on %s: %s" % [name, self.class.name, detail]
                     )
                 end
                 hash.delete name
@@ -898,11 +902,15 @@ class Type < Puppet::Element
     # return the full path to us, for logging and rollback
     # some classes (e.g., FileTypeRecords) will have to override this
     def path
-        if defined? @parent
-            return [@parent.path, self.name].flatten
-        else
-            return [self.name]
+        unless defined? @path
+            if defined? @parent
+                @path = [@parent.path, self.name].flatten.to_s
+            else
+                @path = self.name
+            end
         end
+
+        return @path
     end
 
     # retrieve the current value of all contained states
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index e5ec383f9..2c2e8ab4f 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -297,15 +297,15 @@ module Puppet
                         @children << child
                     rescue Puppet::Error => detail
                         self.notice(
-                            "Cannot manage %s: %s" %
-                                [path,detail.message]
+                            "Cannot manage: %s" %
+                                [detail.message]
                         )
                         self.debug args.inspect
                         child = nil
                     rescue => detail
                         self.notice(
-                            "Cannot manage %s: %s" %
-                                [path,detail]
+                            "Cannot manage: %s" %
+                                [detail]
                         )
                         self.debug args.inspect
                         child = nil
diff --git a/lib/puppet/type/pfile/uid.rb b/lib/puppet/type/pfile/uid.rb
index 298be997a..b61fa8af7 100755
--- a/lib/puppet/type/pfile/uid.rb
+++ b/lib/puppet/type/pfile/uid.rb
@@ -101,8 +101,7 @@ module Puppet
                     @parent.stat(true)
                     self.retrieve
                     if @is == :notfound
-                        self.info "File does not exist; cannot set owner" %
-                            @parent[:path]
+                        self.info "File does not exist; cannot set owner"
                         return nil
                     end
                     if self.insync?
@@ -114,8 +113,8 @@ module Puppet
                 begin
                     File.chown(self.should,nil,@parent[:path])
                 rescue => detail
-                    raise Puppet::Error, "Failed to set owner of '%s' to '%s': %s" %
-                        [@parent[:path],self.should,detail]
+                    raise Puppet::Error, "Failed to set owner to '%s': %s" %
+                        [self.should,detail]
                 end
 
                 return :inode_changed
diff --git a/lib/puppet/type/state.rb b/lib/puppet/type/state.rb
index 5af4e38d1..c30a4f4d1 100644
--- a/lib/puppet/type/state.rb
+++ b/lib/puppet/type/state.rb
@@ -26,7 +26,7 @@ class State < Puppet::Element
         @is = nil
 
         unless hash.include?(:parent)
-            raise "State %s was not passed a parent" % self
+            raise Puppet::DevError, "State %s was not passed a parent" % self
         end
         @parent = hash[:parent]