Lots of refactoring, and added the capture_stderr method

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

10 files changed, 150 insertions, 63 deletions

diff --git a/lib/puppet/element.rb b/lib/puppet/element.rb
index 3991160db..4d1ad8bac 100644
--- a/lib/puppet/element.rb
+++ b/lib/puppet/element.rb
@@ -25,19 +25,7 @@ class Puppet::Element
         }
     }
 
-    # create instance methods for each of the log levels, too
-    Puppet::Log.eachlevel { |level|
-        define_method(level,proc { |args|
-            if args.is_a?(Array)
-                args = args.join(" ")
-            end
-            Puppet::Log.create(
-                :level => level,
-                :source => self,
-                :message => args
-            )
-        })
-    }
+    Puppet::Util.logmethods(self, true)
 
     # for testing whether we should actually do anything
     def noop
@@ -66,8 +54,11 @@ class Puppet::Element
                 else
                     # We assume that if we don't have a parent that we should not
                     # cache the path
-                    Puppet.warning "%s has no parent" % self.name
-                    @path = [self.class.name.to_s + "=" + self.name]
+                    if self.is_a?(Puppet::Type::Component)
+                        @path = [self.name]
+                    else
+                        @path = [self.class.name.to_s + "=" + self.name]
+                    end
                 end
             end
         end
diff --git a/lib/puppet/log.rb b/lib/puppet/log.rb
index 6120df0ac..9fff07225 100644
--- a/lib/puppet/log.rb
+++ b/lib/puppet/log.rb
@@ -183,7 +183,7 @@ module Puppet # :nodoc:
                         ] + RESET
                     else
                         puts @colors[msg.level] + "%s: %s: %s" % [
-                            msg.source, msg.level, msg.to_s
+                            msg.level, msg.source, msg.to_s
                         ] + RESET
                     end
                 when Puppet::Client::LogClient
@@ -294,9 +294,13 @@ module Puppet # :nodoc:
             Log.newmessage(self)
 		end
 
+        # If they pass a source in to us, we make sure it is a string, and
+        # we retrieve any tags we can.
         def source=(source)
-            # We can't store the actual source, we just store the path
-            if source.respond_to?(:path)
+            # We can't store the actual source, we just store the path.  This
+            # is a bit of a stupid hack, specifically testing for elements, but
+            # eh.
+            if source.is_a?(Puppet::Element) and source.respond_to?(:path)
                 @source = source.path
             else
                 @source = source.to_s
diff --git a/lib/puppet/server/authstore.rb b/lib/puppet/server/authstore.rb
index 075f1f9a1..9c8f700dc 100755
--- a/lib/puppet/server/authstore.rb
+++ b/lib/puppet/server/authstore.rb
@@ -19,6 +19,8 @@ class Server
             :name => [:hostname, :domain]
         }
 
+        Puppet::Util.logmethods(self, false)
+
         def allow(pattern)
             # a simple way to allow anyone at all to connect
             if pattern == "*"
@@ -66,7 +68,7 @@ class Server
                 }
             }
 
-            Puppet.info "Defaulting to false for %s" % name
+            self.info "Defaulting to false for %s" % name
             # default to false
             return false
         end
diff --git a/lib/puppet/server/fileserver.rb b/lib/puppet/server/fileserver.rb
index f792c7cd3..92bc129ec 100755
--- a/lib/puppet/server/fileserver.rb
+++ b/lib/puppet/server/fileserver.rb
@@ -19,6 +19,8 @@ class Server
             iface.add_method("string retrieve(string)")
         }
 
+        # Run 'retrieve' on a file.  This gets the actual parameters, so
+        # we can pass them to the client.
         def check(dir)
             unless FileTest.exists?(dir)
                 Puppet.notice "File source %s does not exist" % dir
@@ -41,17 +43,19 @@ class Server
             return obj
         end
 
+        # Describe a given file.  This returns all of the manageable aspects
+        # of that file.
         def describe(file, client = nil, clientip = nil)
             readconfig
             mount, path = splitpath(file)
 
-            unless @mounts[mount].allowed?(client, clientip)
+            unless mount.allowed?(client, clientip)
                 raise Puppet::Server::AuthorizationError, "Cannot access %s" % mount
             end
 
             sdir = nil
             unless sdir = subdir(mount, path)
-                Puppet.notice "Could not find subdirectory %s" %
+                mount.notice "Could not find subdirectory %s" %
                     "//%s/%s" % [mount, path]
                 return ""
             end
@@ -65,13 +69,13 @@ class Server
             CHECKPARAMS.each { |check|
                 if state = obj.state(check)
                     unless state.is
-                        Puppet.notice "Manually retrieving info for %s" % check
+                        mount.notice "Manually retrieving info for %s" % check
                         state.retrieve
                     end
                     desc << state.is
                 else
                     if check == "checksum" and obj.state(:type).is == "file"
-                        Puppet.notice "File %s does not have data for %s" %
+                        mount.notice "File %s does not have data for %s" %
                             [obj.name, check]
                     end
                     desc << nil
@@ -81,6 +85,7 @@ class Server
             return desc.join("\t")
         end
 
+        # Deal with ignore parameters.
         def handleignore(children, path, ignore)            
             ignore.each { |ignore|                
                 Dir.glob(File.join(path,ignore), File::FNM_DOTMATCH) { |match|
@@ -90,6 +95,7 @@ class Server
             return children
         end  
 
+        # Create a new fileserving module.
         def initialize(hash = {})
             @mounts = {}
             @files = {}
@@ -129,18 +135,19 @@ class Server
             end
         end
 
+        # List a specific directory's contents.
         def list(dir, recurse = false, ignore = false, client = nil, clientip = nil)
             readconfig
             mount, path = splitpath(dir)
 
-            unless @mounts[mount].allowed?(client, clientip)
+            unless mount.allowed?(client, clientip)
                 raise Puppet::Server::AuthorizationError, "Cannot access %s" % mount
             end
 
             subdir = nil
             unless subdir = subdir(mount, path)
-                Puppet.notice "Could not find subdirectory %s" %
-                    "//%s/%s" % [mount, path]
+                mount.notice "Could not find subdirectory %s" %
+                    "%s:%s" % [mount, path]
                 return ""
             end
 
@@ -149,12 +156,11 @@ class Server
                 return ""
             end
 
-            #rmdir = File.dirname(File.join(@mounts[mount], path))
-            rmdir = nameswap(dir, mount)
-            desc = reclist(rmdir, subdir, recurse, ignore)
+            rmdir = expand_mount(dir, mount)
+            desc = reclist(mount, rmdir, subdir, recurse, ignore)
 
             if desc.length == 0
-                Puppet.notice "Got no information on //%s/%s" %
+                mount.notice "Got no information on //%s/%s" %
                     [mount, path]
                 return ""
             end
@@ -164,6 +170,7 @@ class Server
             }.join("\n")
         end
 
+        # Mount a new directory with a name.
         def mount(path, name)
             if @mounts.include?(name)
                 if @mounts[name] != path
@@ -178,7 +185,7 @@ class Server
             if FileTest.directory?(path)
                 if FileTest.readable?(path)
                     @mounts[name] = Mount.new(name, path)
-                    Puppet.info "Mounted %s at %s" % [path, name]
+                    @mounts[name].info "Mounted"
                 else
                     raise FileServerError, "%s is not readable" % path
                 end
@@ -187,6 +194,7 @@ class Server
             end
         end
 
+        # Read the configuration file.
         def readconfig
             return if @noreadconfig
 
@@ -237,8 +245,7 @@ class Server
                             when "allow":
                                 value.split(/\s*,\s*/).each { |val|
                                     begin
-                                        Puppet.info "Allowing %s access to %s" %
-                                            [val, mount.name]
+                                        mount.info "Allowing %s access" % val
                                         mount.allow(val)
                                     rescue AuthStoreError => detail
                                         raise FileServerError, "%s at line %s of %s" %
@@ -248,8 +255,7 @@ class Server
                             when "deny":
                                 value.split(/\s*,\s*/).each { |val|
                                     begin
-                                        Puppet.info "Denying %s access to %s" %
-                                            [val, mount.name]
+                                        mount.info "Denying %s access" % val
                                         mount.deny(val)
                                     rescue AuthStoreError => detail
                                         raise FileServerError, "%s at line %s of %s" %
@@ -289,24 +295,21 @@ class Server
             @configstatted = Time.now
         end
 
+        # Retrieve a file from the local disk and pass it to the remote
+        # client.
         def retrieve(file, client = nil, clientip = nil)
             readconfig
             mount, path = splitpath(file)
 
-            unless (@mounts.include?(mount))
-                raise Puppet::Server::FileServerError,
-                    "FileServer module '%s' not mounted" % mount
-            end
-
-            unless @mounts[mount].allowed?(client, clientip)
+            unless mount.allowed?(client, clientip)
                 raise Puppet::Server::AuthorizationError, "Cannot access %s" % mount
             end
 
             fpath = nil
             if path
-                fpath = File.join(@mounts[mount].path, path)
+                fpath = File.join(mount.path, path)
             else
-                fpath = @mounts[mount].path
+                fpath = mount.path
             end
 
             unless FileTest.exists?(fpath)
@@ -324,24 +327,26 @@ class Server
 
         private
 
-        def nameswap(name, mount)
-            name.sub(/\/#{mount}/, @mounts[mount].path).gsub(%r{//}, '/').sub(
+        # Convert from the '/mount/path' form to the real path on disk.
+        def expand_mount(name, mount)
+            # Note that the user could have passed a path with multiple /'s
+            # in it, and we are likely to result in multiples, so we have to
+            # get rid of all of them.
+            name.sub(/\/#{mount.name}/, mount.path).gsub(%r{/+}, '/').sub(
                 %r{/$}, ''
             )
-            #Puppet.info "Swapped %s to %s" % [name, newname]
-            #newname
         end
 
-        # recursive listing function
-        def reclist(root, path, recurse, ignore)
-            #desc = [obj.name.sub(%r{#{root}/?}, '')]
+        # Recursively list the directory.
+        def reclist(mount, root, path, recurse, ignore)
+            # Take out the root of the path.
             name = path.sub(root, '')
             if name == ""
                 name = "/"
             end
 
             if name == path
-                raise Puppet::FileServerError, "Could not match %s in %s" %
+                raise FileServerError, "Could not match %s in %s" %
                     [root, path]
             end
 
@@ -362,7 +367,7 @@ class Server
                     end  
                     children.each { |child|
                         next if child =~ /^\.\.?$/
-                        reclist(root, File.join(path, child), recurse, ignore).each { |cobj|
+                        reclist(mount, root, File.join(path, child), recurse, ignore).each { |cobj|
                             ary << cobj
                         }
                     }
@@ -372,6 +377,7 @@ class Server
             return ary.reject { |c| c.nil? }
         end
 
+        # Split the path into the separate mount point and path.
         def splitpath(dir)
             # the dir is based on one of the mounts
             # so first retrieve the mount path
@@ -389,6 +395,9 @@ class Server
                     raise FileServerError,
                         "Fileserver error: Mount '%s' does not have a path set" % mount
                 end
+
+                # And now replace the name with the actual object.
+                mount = @mounts[mount]
             else
                 raise FileServerError, "Fileserver error: Invalid path '%s'" % dir
             end
@@ -399,8 +408,9 @@ class Server
             return mount, path
         end
 
+        # Retrieve a specific directory relative to a mount point.
         def subdir(mount, dir)
-            basedir = @mounts[mount].path
+            basedir = mount.path
 
             dirname = nil
             if dir
@@ -409,12 +419,18 @@ class Server
                 dirname = basedir
             end
 
-            return dirname
+            dirname
         end
 
+        # A simple class for wrapping mount points.  Instances of this class
+        # don't know about the enclosing object; they're mainly just used for
+        # authorization.
         class Mount < AuthStore
             attr_reader :path, :name
 
+            Puppet::Util.logmethods(self, true)
+
+            # Create out orbject.  It must have a name.
             def initialize(name, path = nil)
                 unless name =~ %r{^\w+$}
                     raise FileServerError, "Invalid name format '%s'" % name
@@ -430,6 +446,7 @@ class Server
                 super()
             end
 
+            # Set the path.
             def path=(path)
                 unless FileTest.exists?(path)
                     raise FileServerError, "%s does not exist" % path
@@ -438,7 +455,11 @@ class Server
             end
 
             def to_s
-                @path
+                if @path
+                    @name + ":" + @path
+                else
+                    @name
+                end
             end
 
             # Verify our configuration is valid.  This should really check to
diff --git a/lib/puppet/storage.rb b/lib/puppet/storage.rb
index dfcb5fe55..8f2d12de6 100644
--- a/lib/puppet/storage.rb
+++ b/lib/puppet/storage.rb
@@ -31,6 +31,7 @@ module Puppet
                 Puppet.info "Statefile %s does not exist" % Puppet[:checksumfile]
                 return
             end
+            begin
             #Puppet.debug "Loading statefile %s" % Puppet[:checksumfile]
 			File.open(Puppet[:checksumfile]) { |file|
 				file.each { |line|
@@ -46,6 +47,9 @@ module Puppet
                     end
 				}
 			}
+            rescue => detail
+                Puppet.err "Could not read %s" % Puppet[:checksumfile]
+            end
 
             #Puppet.debug "Loaded state is %s" % @@state.inspect
 		end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 5282076d5..a71104fc0 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -349,6 +349,14 @@ class Type < Puppet::Element
         return @validstates.keys
     end
 
+    # Return the state class associated with a name
+    def self.statebyname(name)
+        unless @validstates.length == @states.length
+            self.buildstatehash
+        end
+        @validstates[name]
+    end
+
     # does the name reflect a valid parameter?
     def self.validparameter?(name)
         unless defined? @parameters
diff --git a/lib/puppet/type/exec.rb b/lib/puppet/type/exec.rb
index fff3aeee2..49f3e8c4c 100755
--- a/lib/puppet/type/exec.rb
+++ b/lib/puppet/type/exec.rb
@@ -92,11 +92,20 @@ module Puppet
                         # handlers correctly
                         Puppet::Util.asuser(@parent[:user], @parent[:group]) {
                             # capture both stdout and stderr
-                            @output = %x{#{self.parent[:command]} 2>&1}
+
+                            stderr = Puppet::Util.capture_stderr {
+                                @output = %x{#{self.parent[:command]}}
+                            }
+
+                            if stderr != ""
+                                stderr.split(/\n/).each { |line|
+                                    self.send(:err, line)
+                                }
+                            end
                         }
                         status = $?
 
-                        loglevel = :info
+                        loglevel = @parent[:loglevel]
                         if status.exitstatus.to_s != self.should.to_s
                             err("%s returned %s" %
                                 [self.parent[:command],status.exitstatus])
@@ -157,7 +166,17 @@ module Puppet
             @doc = "Executes external commands.  It is critical that all commands
                 executed using this mechanism can be run multiple times without
                 harm, i.e., they are *idempotent*.  One useful way to create idempotent
-                commands is to use the *creates* parameter."
+                commands is to use the *creates* parameter.
+
+                It is worth nothing that ``exec`` is special, in that it is not
+                currently considered an error to have multiple ``exec`` instances
+                with the same name.  This was done purely because it had to be this
+                way in order to get certain functionality, but it complicates things.
+                In particular, you will not be able to use ``exec`` instances that
+                share their commands with other instances as a dependency, since
+                Puppet has no way of knowing which instance you mean.
+
+                It is recommended to avoid duplicate names whenever possible."
             @name = :exec
             @namevar = :command
 
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index ff097fe64..3d5a87011 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -471,7 +471,7 @@ module Puppet
                 end
 
                 unless stat = self.stat(true)
-                    self.debug "File does not exist" % self.name
+                    self.debug "File does not exist"
                     @states.each { |name,state|
                         # We've already retreived the source, and we don't
                         # want to overwrite whatever it did.  This is a bit
diff --git a/lib/puppet/type/service.rb b/lib/puppet/type/service.rb
index a89d56e12..24b5a2492 100644
--- a/lib/puppet/type/service.rb
+++ b/lib/puppet/type/service.rb
@@ -73,8 +73,7 @@ module Puppet
 
             def retrieve
                 self.is = @parent.status
-                self.debug "Running value is '%s'" %
-                    [self.parent.name,self.is]
+                self.debug "Running value is '%s'" % self.is
             end
 
             def sync
@@ -201,13 +200,14 @@ module Puppet
                 end
                 ps = Facter["ps"].value
                 unless ps and ps != ""
-                    raise Puppet::Error, "You must upgrade Facter"
+                    raise Puppet::Error,
+                        "You must upgrade Facter to a version that includes 'ps'"
                 end
                 regex = Regexp.new(self[:pattern])
                 IO.popen(ps) { |table|
                     table.each { |line|
                         if regex.match(line)
-                            ary = line.split(/\s+/)
+                            ary = line.sub(/^\s+/, '').split(/\s+/)
                             return ary[1]
                         end
                     }
@@ -230,6 +230,7 @@ module Puppet
                 end
 
                 # Extend the object with the service type
+                #self.info "extending with %s" % type
                 self.extend(type)
 
                 super
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 50f488905..6169bde81 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -85,6 +85,43 @@ module Util
         return retval
     end
 
+    # Capture stderr of a block
+    def self.capture_stderr
+        require 'stringio'
+        begin
+            $stderr = StringIO.new
+            yield
+            $stderr.rewind && $stderr.read
+        ensure
+            $stderr = STDERR
+        end
+    end
+
+    # Create instance methods for each of the log levels.  This allows
+    # the messages to be a little richer.  Most classes will be calling this
+    # method.
+    def self.logmethods(klass, useself = true)
+        Puppet::Log.eachlevel { |level|
+            klass.send(:define_method, level, proc { |args|
+                if args.is_a?(Array)
+                    args = args.join(" ")
+                end
+                if useself
+                    Puppet::Log.create(
+                        :level => level,
+                        :message => args
+                    )
+                else
+                    Puppet::Log.create(
+                        :level => level,
+                        :source => self,
+                        :message => args
+                    )
+                end
+            })
+        }
+    end
+
     # XXX this should all be done using puppet objects, not using
     # normal mkdir
     def self.recmkdir(dir,mode = 0755)