I have not yet finished testing, but most of the providers now successfully pass arrays to execute() instead of strings, which means that the vast majority of execution problems are now gone. I will finish testing tomorrow, hopefully, and will also hopefully be able to verify that the execution-related bugs are fixed.

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

35 files changed, 197 insertions, 176 deletions

diff --git a/lib/puppet/client/pelement.rb b/lib/puppet/client/pelement.rb
index 1dda36ddc..858bce811 100644
--- a/lib/puppet/client/pelement.rb
+++ b/lib/puppet/client/pelement.rb
@@ -28,7 +28,7 @@ class Puppet::Client::PElement < Puppet::Client
     end
 
     def describe(type, name, retrieve = false, ignore = false)
-        Puppet.info "Describing %s[%s]" % [type, name]
+        Puppet.info "Describing %s[%s]" % [type.capitalize, name]
         text = @driver.describe(type, name, retrieve, ignore, "yaml")
 
         object = nil
diff --git a/lib/puppet/provider/group/groupadd.rb b/lib/puppet/provider/group/groupadd.rb
index fe6150bd2..c37bdb933 100644
--- a/lib/puppet/provider/group/groupadd.rb
+++ b/lib/puppet/provider/group/groupadd.rb
@@ -14,7 +14,7 @@ Puppet::Type.type(:group).provide :groupadd, :parent => Puppet::Provider::NameSe
         cmd = [command(:add)]
         if gid = @model.should(:gid)
             unless gid == :absent
-                cmd << flag(:gid) << "'%s'" % gid
+                cmd << flag(:gid) << gid
             end
         end
         if @model[:allowdupe] == :true
@@ -22,7 +22,7 @@ Puppet::Type.type(:group).provide :groupadd, :parent => Puppet::Provider::NameSe
         end
         cmd << @model[:name]
 
-        return cmd.join(" ")
+        return cmd
     end
 end
 
diff --git a/lib/puppet/provider/group/pw.rb b/lib/puppet/provider/group/pw.rb
index 4cfcf997a..8011357b5 100644
--- a/lib/puppet/provider/group/pw.rb
+++ b/lib/puppet/provider/group/pw.rb
@@ -14,7 +14,7 @@ Puppet::Type.type(:group).provide :pw, :parent => Puppet::Provider::NameService:
         cmd = [command(:pw), "groupadd", @model[:name]]
         if gid = @model.should(:gid)
             unless gid == :absent
-                cmd << flag(:gid) << "'%s'" % gid
+                cmd << flag(:gid) << gid
             end
         end
 
@@ -24,7 +24,7 @@ Puppet::Type.type(:group).provide :pw, :parent => Puppet::Provider::NameService:
         #    cmd << "-o"
         #end
 
-        return cmd.join(" ")
+        return cmd
     end
 end
 
diff --git a/lib/puppet/provider/mount.rb b/lib/puppet/provider/mount.rb
index d1ba79c7c..fc3e5acef 100644
--- a/lib/puppet/provider/mount.rb
+++ b/lib/puppet/provider/mount.rb
@@ -19,12 +19,12 @@ module Puppet::Provider::Mount
     # Is the mount currently mounted?
     def mounted?
         platform = Facter["operatingsystem"].value
-        df = command(:df)
+        df = [command(:df)]
         case Facter["operatingsystem"].value
         # Solaris's df prints in a very weird format
-        when "Solaris": df = "#{command(:df)} -k"
+        when "Solaris": df << "-k"
         end
-        %x{#{df}}.split("\n").find do |line|
+        execute(df).split("\n").find do |line|
             fs = line.split(/\s+/)[-1]
             if platform == "Darwin"
                 fs == "/private/var/automount" + @model[:name] or
diff --git a/lib/puppet/provider/mount/netinfo.rb b/lib/puppet/provider/mount/netinfo.rb
index 738f8269d..9cebc11e7 100644
--- a/lib/puppet/provider/mount/netinfo.rb
+++ b/lib/puppet/provider/mount/netinfo.rb
@@ -31,7 +31,7 @@ Puppet::Type.type(:mount).provide :netinfo, :parent => Puppet::Provider::NameSer
         end
         cmd << @model.should(:device)
         cmd << @model[:name]
-        mountcmd cmd.join(" ")
+        mountcmd cmd
     end
 end
 
diff --git a/lib/puppet/provider/nameservice/netinfo.rb b/lib/puppet/provider/nameservice/netinfo.rb
index 54f2e8c00..8f1a4ee07 100644
--- a/lib/puppet/provider/nameservice/netinfo.rb
+++ b/lib/puppet/provider/nameservice/netinfo.rb
@@ -9,10 +9,17 @@ class NetInfo < Puppet::Provider::NameService
     class << self
         attr_writer :netinfodir
     end
+
+    # We have to initialize manually because we're not using
+    # classgen() here.
+    initvars()
+
+    commands :lookupd => "/usr/sbin/lookupd"
+
     # Attempt to flush the database, but this doesn't seem to work at all.
     def self.flush
         begin
-            output = execute(["/usr/sbin/lookupd", "-flushcache"])
+            lookupd "-flushcache"
         rescue Puppet::ExecutionFailure
             # Don't throw an error; it's just a failed cache flush
             Puppet.err "Could not flush lookupd cache: %s" % output
@@ -109,8 +116,7 @@ class NetInfo < Puppet::Provider::NameService
         cmd = [command(:niutil)]
         cmd << arg
 
-        cmd << "/" << "/%s/%s" %
-            [self.class.netinfodir(), @model[:name]]
+        cmd << "/" << "/%s/%s" % [self.class.netinfodir(), @model[:name]]
         return cmd
     end
 
@@ -172,8 +178,7 @@ class NetInfo < Puppet::Provider::NameService
         #     warning "Netinfo providers cannot currently handle multiple values"
         # end
 
-        cmd << "-createprop" << "/" << "/%s/%s" %
-            [self.class.netinfodir, @model[:name]]
+        cmd << "-createprop" << "/" << "/%s/%s" % [self.class.netinfodir, @model[:name]]
 
         value = [value] unless value.is_a?(Array)
         if key = netinfokey(param)
diff --git a/lib/puppet/provider/nameservice/objectadd.rb b/lib/puppet/provider/nameservice/objectadd.rb
index 056ac5e84..af099289f 100644
--- a/lib/puppet/provider/nameservice/objectadd.rb
+++ b/lib/puppet/provider/nameservice/objectadd.rb
@@ -12,7 +12,7 @@ class ObjectAdd < Puppet::Provider::NameService
     end
     
     def deletecmd
-        [command(:delete), @model[:name]].join(" ")
+        [command(:delete), @model[:name]]
     end
 
     # Determine the flag to pass to our command.
@@ -24,13 +24,13 @@ class ObjectAdd < Puppet::Provider::NameService
     def modifycmd(param, value)
         cmd = [command(:modify),
             flag(param),
-            "'%s'" % value]
+            value]
         if @model[:allowdupe]  == :true
             cmd << "-o"
         end
         cmd << @model[:name]
 
-        return cmd.join(" ")
+        return cmd
     end
 
     def posixmethod(name)
diff --git a/lib/puppet/provider/nameservice/pw.rb b/lib/puppet/provider/nameservice/pw.rb
index e151a258d..72d7628e6 100644
--- a/lib/puppet/provider/nameservice/pw.rb
+++ b/lib/puppet/provider/nameservice/pw.rb
@@ -3,7 +3,7 @@ require 'puppet/provider/nameservice/objectadd'
 class Puppet::Provider::NameService
 class PW < ObjectAdd
     def deletecmd
-        "#{command(:pw)} #{@model.class.name.to_s}del %s" % @model[:name]
+        [command(:pw), "#{@model.class.name.to_s}del", @model[:name]]
     end
 
     def modifycmd(param, value)
@@ -12,9 +12,9 @@ class PW < ObjectAdd
             "#{@model.class.name.to_s}mod",
             @model[:name],
             flag(param),
-            "'%s'" % value
+            value
         ]
-        return cmd.join(" ")
+        return cmd
     end
 end
 end
diff --git a/lib/puppet/provider/package/apt.rb b/lib/puppet/provider/package/apt.rb
index 511687338..275e96222 100755
--- a/lib/puppet/provider/package/apt.rb
+++ b/lib/puppet/provider/package/apt.rb
@@ -16,8 +16,8 @@ Puppet::Type.type(:package).provide :apt, :parent => :dpkg do
     # Debian boxes, and the only thing that differs is that it can
     # install packages from remote sites.
 
-    def aptcmd(arg)
-        aptget(arg)
+    def aptcmd(*args)
+        aptget(*args)
     end
 
     def checkforcdrom
@@ -59,25 +59,28 @@ Puppet::Type.type(:package).provide :apt, :parent => :dpkg do
             # Add the package version
             str += "=%s" % should
         end
+        cmd = %w{-q -y}
 
         keep = ""
         if config = @model[:configfiles]
             case config
             when :keep
-                keep = "-o 'DPkg::Options::=--force-confold'"
+                cmd << "-o" << 'DPkg::Options::=--force-confold'
             when :replace
-                keep = "-o 'DPkg::Options::=--force-confnew'"
+                cmd << "-o" << 'DPkg::Options::=--force-confnew'
             else
                 raise Puppet::Error, "Invalid 'configfiles' value %s" % config
             end
         end
+
+        cmd << :install << str
         
-        aptcmd("-q -y %s install %s" % [keep, str])
+        aptcmd(cmd)
     end
 
     # What's the latest package version available?
     def latest
-        output = aptcache("showpkg %s" % @model[:name] )
+        output = aptcache :showpkg,  @model[:name]
 
         if output =~ /Versions:\s*\n((\n|.)+)^$/
             versions = $1
@@ -109,7 +112,7 @@ Puppet::Type.type(:package).provide :apt, :parent => :dpkg do
 	# preseeds answers to dpkg-set-selection from the "responsefile"
 	#
     def run_preseed
-        if response = @model[:responsefile] && FileTest.exists?(response)
+        if response = @model[:responsefile] and FileTest.exists?(response)
             self.info("Preseeding %s to debconf-set-selections" % response)
 
             preseed response
@@ -123,7 +126,7 @@ Puppet::Type.type(:package).provide :apt, :parent => :dpkg do
     end
 
     def uninstall
-        aptcmd("-y -q remove %s" % @model[:name])
+        aptcmd "-y", "-q", :remove, @model[:name]
     end
 
     def versionable?
diff --git a/lib/puppet/provider/package/aptitude.rb b/lib/puppet/provider/package/aptitude.rb
index 7b74acedc..68cb13b63 100755
--- a/lib/puppet/provider/package/aptitude.rb
+++ b/lib/puppet/provider/package/aptitude.rb
@@ -6,12 +6,15 @@ Puppet::Type.type(:package).provide :aptitude, :parent => :apt do
 
     ENV['DEBIAN_FRONTEND'] = "noninteractive"
 
-    def aptcmd(arg)
+    def aptcmd(*args)
         # Apparently aptitude hasn't always supported a -q flag.
-        output = aptitude(arg.gsub(/-q/,""))
+        if args.include?("-q")
+            args.delete("-q")
+        end
+        output = aptitude(*args)
 
         # Yay, stupid aptitude doesn't throw an error when the package is missing.
-        if output =~ /0 newly installed/
+        if args.include?(:install) and output =~ /0 newly installed/
             raise Puppet::Error.new(
                 "Could not find package %s" % self.name
             )
diff --git a/lib/puppet/provider/package/blastwave.rb b/lib/puppet/provider/package/blastwave.rb
index 5add9705d..61c5bc1da 100755
--- a/lib/puppet/provider/package/blastwave.rb
+++ b/lib/puppet/provider/package/blastwave.rb
@@ -32,10 +32,10 @@ Puppet::Type.type(:package).provide :blastwave, :parent => :sun do
 
     # Turn our blastwave listing into a bunch of hashes.
     def self.blastlist(hash)
-        command = "-c"
+        command = ["-c"]
 
         if hash[:justme]
-            command += " " + hash[:justme]
+            command << hash[:justme]
         end
 
         output = pkgget command
@@ -84,7 +84,7 @@ Puppet::Type.type(:package).provide :blastwave, :parent => :sun do
     end
 
     def install
-        pkgget "-f install #{@model[:name]}"
+        pkgget "-f", :install, @model[:name]
     end
 
     # Retrieve the version from the current package file.
@@ -101,11 +101,11 @@ Puppet::Type.type(:package).provide :blastwave, :parent => :sun do
 
     # Remove the old package, and install the new one
     def update
-        pkgget "-f upgrade #{@model[:name]}"
+        pkgget "-f", :upgrade, @model[:name]
     end
 
     def uninstall
-        pkgget "-f remove #{@model[:name]}"
+        pkgget "-f", :remove, @model[:name]
     end
 end
 
diff --git a/lib/puppet/provider/package/darwinport.rb b/lib/puppet/provider/package/darwinport.rb
index 729371db5..36d87456f 100755
--- a/lib/puppet/provider/package/darwinport.rb
+++ b/lib/puppet/provider/package/darwinport.rb
@@ -46,7 +46,7 @@ Puppet::Type.type(:package).provide :darwinport do
         should = @model.should(:ensure)
 
         # Seems like you can always say 'upgrade'
-        port "upgrade #{@model[:name]}"
+        port "upgrade", @model[:name]
     end
 
     def query
@@ -61,7 +61,7 @@ Puppet::Type.type(:package).provide :darwinport do
     end
 
     def latest
-        info = port "search '^#{@model[:name]}$' 2>/dev/null"
+        info = port :search, "^#{@model[:name]}$"
 
         if $? != 0 or info =~ /^Error/
             return nil
@@ -74,7 +74,7 @@ Puppet::Type.type(:package).provide :darwinport do
     end
 
     def uninstall
-        port "uninstall #{@model[:name]}"
+        port :uninstall, @model[:name]
     end
 
     def update
diff --git a/lib/puppet/provider/package/dpkg.rb b/lib/puppet/provider/package/dpkg.rb
index f747a5a0c..0e49f84aa 100755
--- a/lib/puppet/provider/package/dpkg.rb
+++ b/lib/puppet/provider/package/dpkg.rb
@@ -87,7 +87,7 @@ Puppet::Type.type(:package).provide :dpkg do
     end
 
     def uninstall
-        dpkg "-r %s" % @model[:name]
+        dpkg "-r", @model[:name]
     end
 end
 
diff --git a/lib/puppet/provider/package/freebsd.rb b/lib/puppet/provider/package/freebsd.rb
index 5474ad5ca..f1649efa4 100755
--- a/lib/puppet/provider/package/freebsd.rb
+++ b/lib/puppet/provider/package/freebsd.rb
@@ -8,6 +8,8 @@ Puppet::Type.type(:package).provide :freebsd, :parent => :openbsd do
     commands :pkginfo => "/usr/sbin/pkg_info",
              :pkgadd => "/usr/sbin/pkg_add",
              :pkgdelete => "/usr/sbin/pkg_delete"
+    
+    confine :operatingsystem => :freebsd
 
     def self.listcmd
         command(:pkginfo)
@@ -20,7 +22,7 @@ Puppet::Type.type(:package).provide :freebsd, :parent => :openbsd do
             return super
         end
 
-        pkgadd " -r " + @model[:name]
+        pkgadd "-r", @model[:name]
     end
 
     def query
diff --git a/lib/puppet/provider/package/openbsd.rb b/lib/puppet/provider/package/openbsd.rb
index b0d5a2277..2d1b6d07d 100755
--- a/lib/puppet/provider/package/openbsd.rb
+++ b/lib/puppet/provider/package/openbsd.rb
@@ -5,6 +5,7 @@ Puppet::Type.type(:package).provide :openbsd do
     commands :pkginfo => "pkg_info", :pkgadd => "pkg_add", :pkgdelete => "pkg_delete"
 
     defaultfor :operatingsystem => :openbsd
+    confine :operatingsystem => :openbsd
 
     def self.list
         packages = []
@@ -51,7 +52,7 @@ Puppet::Type.type(:package).provide :openbsd do
     end
 
     def self.listcmd
-        "#{command(:info)} -a"
+        [command(:info), "-a"]
     end
 
     def install
diff --git a/lib/puppet/provider/package/pkgdmg.rb b/lib/puppet/provider/package/pkgdmg.rb
index 2997bea65..e325840b1 100644
--- a/lib/puppet/provider/package/pkgdmg.rb
+++ b/lib/puppet/provider/package/pkgdmg.rb
@@ -44,7 +44,7 @@ Puppet::Type.type(:package).provide :pkgdmg do
     end
 
     def self.installpkg(source, name, orig_source)
-      installer "-pkg '#{source}' -target /"
+      installer "-pkg", source, "-target", "/"
       File.open("/var/db/.puppet_pkgdmg_installed_#{name}", "w") do |t|
           t.print "name: '#{name}'\n"
           t.print "source: '#{orig_source}'\n"
@@ -58,7 +58,7 @@ Puppet::Type.type(:package).provide :pkgdmg do
         require 'open-uri'
         require 'puppet/util/plist'
         open(source) do |dmg|
-            cmd = "/usr/bin/hdiutil mount -plist -nobrowse -readonly -mountrandom /tmp #{dmg.path}"
+            cmd = "#{command(:hdiutil)} mount -plist -nobrowse -readonly -mountrandom /tmp #{dmg.path}"
             IO.popen(cmd) do |pipe|
                 xml_str = pipe.read
                 ptable = Plist::parse_xml xml_str
@@ -73,7 +73,7 @@ Puppet::Type.type(:package).provide :pkgdmg do
                         installpkg("#{fspath}/#{pkg}", name, source)
                     }
                 end
-            hdiutil "eject '#{mounts[0]}'"
+            hdiutil "eject", mounts[0]
             end
         end
     end
diff --git a/lib/puppet/provider/package/portage.rb b/lib/puppet/provider/package/portage.rb
index 3e4cc1e36..a1eeba43c 100644
--- a/lib/puppet/provider/package/portage.rb
+++ b/lib/puppet/provider/package/portage.rb
@@ -5,25 +5,30 @@ Puppet::Type.type(:package).provide :portage do
 
     defaultfor :operatingsystem => :gentoo
 
+    def self.format
+        "{installedversions}<category> <name> [<installedversions>] [<best>] <homepage> <description>{}"
+    end
+
     def self.list
         search_format = /(\S+) (\S+) \[(.*)\] \[([^\s:]*)(:\S*)?\] ([\S]*) (.*)/
         result_fields = [:category, :name, :ensure, :version_available, :slot, :vendor, :description]
-        command = "#{command(:eix)} --format \"{installedversions}<category> <name> [<installedversions>] [<best>] <homepage> <description>{}\""
 
         begin
-            search_output = execute( command )
+            search_output = eix "--format", format()
 
             packages = []
             search_output.each do |search_result|
                 match = search_format.match( search_result )
 
-                if( match )
+                if match
                     package = {}
-                    result_fields.zip( match.captures ) { |field, value| package[field] = value unless !value or value.empty? }
+                    result_fields.zip(match.captures) { |field, value|
+                        package[field] = value unless !value or value.empty?
+                    }
                     package[:provider] = :portage
                     package[:ensure] = package[:ensure].split.last
 
-                    packages.push( Puppet.type(:package).installedpkg(package) )
+                    packages.push(Puppet.type(:package).installedpkg(package))
                 end
             end
 
@@ -34,25 +39,22 @@ Puppet::Type.type(:package).provide :portage do
     end
 
     def install
-        if @model.should( :ensure ) == :present || @model.should( :ensure ) == :latest
-            package_name = "#{@model[:category]}/#{@model[:name]}"
-        else
+        should = @model.should(:ensure)
+        name = package_name
+        unless should == :present or should == :latest
             # We must install a specific version
-            package_name = "=#{@model[:category]}/#{@model[:name]}-#{@model.should( :ensure )}"
+            name = "=%s-%s" % [name, should]
         end
-        command = "#{command(:emerge)} #{package_name}"
+        emerge name
+    end
 
-        output = execute( command )
+    # The common package name format.
+    def package_name
+        "%s/%s" % [@model[:category], @model[:name]]
     end
 
     def uninstall
-        package_name = "#{@model[:category]}/#{@model[:name]}"
-        command ="#{command(:emerge)} --unmerge #{package_name}"
-        begin
-            output = execute( command )
-        rescue Puppet::ExecutionFailure => detail
-            raise Puppet::PackageError.new(detail)
-        end
+        emerge "--unmerge", package_name
     end
 
     def update
@@ -64,10 +66,10 @@ Puppet::Type.type(:package).provide :portage do
         result_fields = [:category, :name, :ensure, :version_available, :slot, :vendor, :description]
 
         search_field = @model[:name].include?( '/' ) ? "--category-name" : "--name"
-        command = "#{command(:eix)} --format \"<category> <name> [<installedversions>] [<best>] <homepage> <description>\" --exact #{search_field} #{@model[:name]}"
+        format = "<category> <name> [<installedversions>] [<best>] <homepage> <description>"
 
         begin
-            search_output = execute( command )
+            search_output = eix "-format", format, "--exact", search_field, @model[:name]
 
             packages = []
             search_output.each do |search_result|
@@ -87,11 +89,11 @@ Puppet::Type.type(:package).provide :portage do
 
             case packages.size
                 when 0
-                    raise Puppet::PackageError.new( "No package found with the specified name [#{@model[:name]}]" )
+                    raise Puppet::PackageError.new("No package found with the specified name [#{@model[:name]}]")
                 when 1
                     return packages[0]
                 else
-                    raise Puppet::PackageError.new( "More than one package with the specified name [#{@model[:name]}], please use category/name to disambiguate" )
+                    raise Puppet::PackageError.new("More than one package with the specified name [#{@model[:name]}], please use category/name to disambiguate")
             end
         rescue Puppet::ExecutionFailure => detail
             raise Puppet::PackageError.new(detail)
diff --git a/lib/puppet/provider/package/ports.rb b/lib/puppet/provider/package/ports.rb
index 8279d1405..685479285 100755
--- a/lib/puppet/provider/package/ports.rb
+++ b/lib/puppet/provider/package/ports.rb
@@ -19,9 +19,9 @@ Puppet::Type.type(:package).provide :ports, :parent => :freebsd do
         # -p: create a package
         # -N: install if the package is missing, otherwise upgrade
         # -P: prefer binary packages
-        cmd = "-p -N -P #{@model[:name]}"
+        cmd = %w{-p -N -P} << @model[:name]
 
-        output = portupgrade cmd
+        output = portupgrade(*cmd)
         if output =~ /\*\* No such /
             raise Puppet::ExecutionFailure, "Could not find package %s" % @model[:name]
         end
@@ -29,10 +29,10 @@ Puppet::Type.type(:package).provide :ports, :parent => :freebsd do
 
     # If there are multiple packages, we only use the last one
     def latest
-        cmd = "-v #{@model[:name]}"
+        cmd = ["-v", @model[:name]]
 
         begin
-            output = portversion(cmd)
+            output = portversion(*cmd)
         rescue Puppet::ExecutionFailure
             raise Puppet::PackageError.new(output)
         end
diff --git a/lib/puppet/provider/package/rpm.rb b/lib/puppet/provider/package/rpm.rb
index ea13ba65e..cda70d10a 100755
--- a/lib/puppet/provider/package/rpm.rb
+++ b/lib/puppet/provider/package/rpm.rb
@@ -47,10 +47,10 @@ Puppet::Type.type(:package).provide :rpm do
             :description => "DESCRIPTION"
         }
 
-        cmd = "-q #{@model[:name]} --qf '%{NAME} #{VERSIONSTRING}\n'"
+        cmd = ["-q", @model[:name], "--qf", '%{NAME} #{VERSIONSTRING}\n']
 
         begin
-            output = rpm cmd
+            output = rpm *cmd
         rescue Puppet::ExecutionFailure
             return nil
         end
@@ -95,11 +95,11 @@ Puppet::Type.type(:package).provide :rpm do
             flag = "-U"
         end
 
-        rpm "#{flag} #{source}"
+        rpm flag, source
     end
 
     def uninstall
-        rpm "-e " + @model[:name]
+        rpm "-e", @model[:name]
     end
 
     def update
diff --git a/lib/puppet/provider/package/sun.rb b/lib/puppet/provider/package/sun.rb
index a2730af2a..5263c5821 100755
--- a/lib/puppet/provider/package/sun.rb
+++ b/lib/puppet/provider/package/sun.rb
@@ -121,16 +121,16 @@ Puppet::Type.type(:package).provide :sun do
         cmd = []
         
         if @model[:adminfile]
-            cmd << " -a " + @model[:adminfile]
+            cmd << "-a" << @model[:adminfile]
         end
 
         if @model[:responsefile]
-            cmd << " -r " + @model[:responsefile]
+            cmd << "-r" << @model[:responsefile]
         end
 
-        cmd += ["-d", @model[:source]]
-        cmd += ["-n", @model[:name]]
-        cmd = cmd.join(" ")
+        cmd << "-d" << @model[:source]
+        cmd << "-n" << @model[:name]
+        cmd = cmd
 
         pkgadd cmd
     end
@@ -146,13 +146,13 @@ Puppet::Type.type(:package).provide :sun do
     end
 
     def uninstall
-        command  = "-n "
+        command  = ["-n"]
 
         if @model[:adminfile]
-            command += " -a " + @model[:adminfile]
+            command << "-a" << @model[:adminfile]
         end
 
-        command += " " + @model[:name]
+        command << @model[:name]
         pkgrm command
     end
 
diff --git a/lib/puppet/provider/package/up2date.rb b/lib/puppet/provider/package/up2date.rb
index 1e1358c4d..85a0ddde6 100644
--- a/lib/puppet/provider/package/up2date.rb
+++ b/lib/puppet/provider/package/up2date.rb
@@ -7,7 +7,7 @@ Puppet.type(:package).provide :up2date, :parent => :rpm do
 
     # Install a package using 'up2date'.
     def install
-        up2date "-u %s" % @model[:name]
+        up2date "-u", @model[:name]
 
         #@states[:ensure].retrieve
         #if @states[:ensure].is == :absent
@@ -21,7 +21,6 @@ Puppet.type(:package).provide :up2date, :parent => :rpm do
     # What's the latest package version available?
     def latest
         #up2date can only get a list of *all* available packages?
-        #cmd = "/usr/sbib/up2date-nox --show-available %s" % self[:name] 
         output = up2date "--show-available"
 
         if output =~ /#{@model[:name]}-(\d+.*)\.\w+/
diff --git a/lib/puppet/provider/package/yum.rb b/lib/puppet/provider/package/yum.rb
index 80f9f7857..0e9559bd0 100755
--- a/lib/puppet/provider/package/yum.rb
+++ b/lib/puppet/provider/package/yum.rb
@@ -6,7 +6,7 @@ Puppet::Type.type(:package).provide :yum, :parent => :rpm do
 
     # Install a package using 'yum'.
     def install
-        output = yum "-y install %s" % @model[:name]
+        output = yum "-y", :install, @model[:name]
 
         unless self.query
             raise Puppet::Error.new(
@@ -17,7 +17,7 @@ Puppet::Type.type(:package).provide :yum, :parent => :rpm do
 
     # What's the latest package version available?
     def latest
-        output = yum "list available %s" % @model[:name] 
+        output = yum :list, :available, @model[:name] 
 
         if output =~ /#{@model[:name]}\S+\s+(\S+)\s/
             return $1
diff --git a/lib/puppet/provider/service/base.rb b/lib/puppet/provider/service/base.rb
index 29f4c2342..89bee741c 100755
--- a/lib/puppet/provider/service/base.rb
+++ b/lib/puppet/provider/service/base.rb
@@ -6,16 +6,7 @@ Puppet::Type.type(:service).provide :base do
         specify start, stop, and status commands, akin to how you would do
         so using ``init``."
 
-    # Execute a command.  Basically just makes sure it exits with a 0
-    # code.
-    def execute(type, cmd)
-        self.debug "Executing %s" % cmd.inspect
-        output = %x(#{cmd} 2>&1)
-        unless $? == 0
-            @model.fail "Could not %s %s: %s" %
-                [type, self.name, output.chomp]
-        end
-    end
+    commands :kill => "kill"
 
     # Get the process ID for a running process. Requires the 'pattern'
     # parameter.
@@ -46,8 +37,7 @@ Puppet::Type.type(:service).provide :base do
     # How to restart the process.
     def restart
         if @model[:restart] or self.respond_to?(:restartcmd)
-            cmd = @model[:restart] || self.restartcmd
-            self.execute("restart", cmd)
+            ucommand(:restart)
         else
             self.stop
             self.start
@@ -63,9 +53,9 @@ Puppet::Type.type(:service).provide :base do
         if @model[:status] or (
             self.respond_to?(:statuscmd) and self.statuscmd
         )
-            cmd = @model[:status] || self.statuscmd
-            self.debug "Executing %s" % cmd.inspect
-            output = %x(#{cmd} 2>&1)
+            # Don't fail when the exit status is not 0.
+            output = ucommand(:status, false)
+
             self.debug "%s status returned %s" %
                 [self.name, output.inspect]
             if $? == 0
@@ -83,8 +73,7 @@ Puppet::Type.type(:service).provide :base do
 
     # Run the 'start' parameter command, or the specified 'startcmd'.
     def start
-        cmd = @model[:start] || self.startcmd
-        self.execute("start", cmd)
+        ucommand(:start)
     end
 
     # The command used to start.  Generated if the 'binary' argument
@@ -107,21 +96,43 @@ Puppet::Type.type(:service).provide :base do
         if @model[:stop]
             return @model[:stop]
         elsif self.respond_to?(:stopcmd)
-            self.execute("stop", self.stopcmd)
+            texecute(:stop, self.stopcmd)
         else
             pid = getpid
             unless pid
                 self.info "%s is not running" % self.name
                 return false
             end
-            output = %x(kill #{pid} 2>&1)
-            if $? != 0
+            begin
+                output = kill pid
+            rescue Puppet::ExecutionFailure => detail
                 @model.fail "Could not kill %s, PID %s: %s" %
                         [self.name, pid, output]
             end
             return true
         end
     end
+
+    # A simple wrapper so execution failures are a bit more informative.
+    def texecute(type, command, fof = true)
+        begin
+            output = execute(command, fof)
+        rescue Puppet::ExecutionFailure => detail
+            warning "Could not %s %s: %s" % [type, @model.ref, detail]
+        end
+
+        return output
+    end
+
+    # Use either a specified command or the default for our provider.
+    def ucommand(type, fof = true)
+        if c = @model[type]
+            cmd = [c]
+        else
+            cmd = self.send("%scmd" % type)
+        end
+        return texecute(type, cmd, fof)
+    end
 end
 
 # $Id$
diff --git a/lib/puppet/provider/service/debian.rb b/lib/puppet/provider/service/debian.rb
index b8acbf326..445108fa9 100755
--- a/lib/puppet/provider/service/debian.rb
+++ b/lib/puppet/provider/service/debian.rb
@@ -9,24 +9,11 @@ Puppet::Type.type(:service).provide :debian, :parent => :init do
 
     # Remove the symlinks
     def disable
-        cmd = %{#{command(:update)} -f #{@model[:name]} remove 2>&1}
-        self.debug "Executing '%s'" % cmd
-        output = %x{#{cmd}}
-
-        unless $? == 0
-            raise Puppet::Error, "Could not disable %s: %s" %
-                [self.name, output]
-        end
+        update "-f", @model[:name], "remove"
     end
 
     def enabled?
-        cmd = %{#{command(:update)} -n -f #{@model[:name]} remove 2>&1}
-        self.debug "Executing 'enabled' test: '%s'" % cmd
-        output = %x{#{cmd}}
-        unless $? == 0
-            raise Puppet::Error, "Could not check %s: %s" %
-                [self.name, output]
-        end
+        output = update "-n", "-f", @model[:name], "remove"
 
         # If it's enabled, then it will print output showing removal of
         # links.
@@ -38,14 +25,7 @@ Puppet::Type.type(:service).provide :debian, :parent => :init do
     end
 
     def enable
-        cmd = %{#{command(:update)} #{@model[:name]} defaults 2>&1}
-        self.debug "Executing '%s'" % cmd
-        output = %x{#{cmd}}
-
-        unless $? == 0
-            raise Puppet::Error, "Could not enable %s: %s" %
-                [self.name, output]
-        end
+        update @model[:name], "defaults"
     end
 end
 
diff --git a/lib/puppet/provider/service/gentoo.rb b/lib/puppet/provider/service/gentoo.rb
index bd3a03f9b..a4d4a9b85 100644
--- a/lib/puppet/provider/service/gentoo.rb
+++ b/lib/puppet/provider/service/gentoo.rb
@@ -10,7 +10,7 @@ Puppet::Type.type(:service).provide :gentoo, :parent => :init do
 
     def disable
         begin
-            output = util_execute("#{command(:update)} del #{@model[:name]} default 2>&1")
+            output = update :del, @model[:name], :default
         rescue Puppet::ExecutionFailure
             raise Puppet::Error, "Could not disable %s: %s" %
                 [self.name, output]
@@ -19,11 +19,15 @@ Puppet::Type.type(:service).provide :gentoo, :parent => :init do
 
     def enabled?
         begin
-            output = util_execute("#{command(:update)} show | grep #{@model[:name]}").chomp
+            output = update :show
         rescue Puppet::ExecutionFailure
             return :false
         end
 
+        line = output.split(/\n/).find { |l| l.include?(@model[:name]) }
+
+        return :false unless line
+
         # If it's enabled then it will print output showing service | runlevel
         if output =~ /#{@model[:name]}\s*|\s*default/
             return :true
@@ -34,7 +38,7 @@ Puppet::Type.type(:service).provide :gentoo, :parent => :init do
 
     def enable
         begin
-            output = util_execute("#{command(:update)} add #{@model[:name]} default 2>&1")
+            output = update :add, @model[:name], :default
         rescue Puppet::ExecutionFailure
             raise Puppet::Error, "Could not enable %s: %s" %
                 [self.name, output]
diff --git a/lib/puppet/provider/service/init.rb b/lib/puppet/provider/service/init.rb
index 90f7c26a7..018aeaae0 100755
--- a/lib/puppet/provider/service/init.rb
+++ b/lib/puppet/provider/service/init.rb
@@ -17,6 +17,7 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
         @defpath = "/etc/init.d"
     end
 
+    # We can't confine this here, because the init path can be overridden.
     #confine :exists => @defpath
 
     if self.suitable?
@@ -92,8 +93,8 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
 
     def restart
         if @model[:hasrestart] == :true
-            command = self.initscript + " restart"
-            self.execute("restart", command)
+            command = [self.initscript, :restart]
+            texecute("restart", command)
         else
             super
         end
@@ -118,7 +119,7 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
 
     # The start command is just the init scriptwith 'start'.
     def startcmd
-        self.initscript + " start"
+        [self.initscript, :start]
     end
 
     # If it was specified that the init script has a 'status' command, then
@@ -126,7 +127,7 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
     # fallback to other mechanisms.
     def statuscmd
         if @model[:hasstatus]
-            return self.initscript + " status"
+            return [self.initscript, :status]
         else
             return false
         end
@@ -134,7 +135,7 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
 
     # The stop command is just the init script with 'stop'.
     def stopcmd
-        self.initscript + " stop"
+        [self.initscript, :stop]
     end
 end
 
diff --git a/lib/puppet/provider/service/redhat.rb b/lib/puppet/provider/service/redhat.rb
index 64983e1dd..93cf2fc73 100755
--- a/lib/puppet/provider/service/redhat.rb
+++ b/lib/puppet/provider/service/redhat.rb
@@ -19,8 +19,8 @@ Puppet::Type.type(:service).provide :redhat, :parent => :init do
     # Remove the symlinks
     def disable
         begin
-            output = util_execute("#{command(:chkconfig)} #{@model[:name]} off 2>&1")
-            output += util_execute("#{command(:chkconfig)} --del #{@model[:name]} 2>&1")
+            output = chkconfig(@model[:name], :off)
+            output += chkconfig("--del", @model[:name])
         rescue Puppet::ExecutionFailure
             raise Puppet::Error, "Could not disable %s: %s" %
                 [self.name, output]
@@ -29,7 +29,7 @@ Puppet::Type.type(:service).provide :redhat, :parent => :init do
 
     def enabled?
         begin
-            output = util_execute("#{command(:chkconfig)} #{@model[:name]} 2>&1").chomp
+            output = chkconfig(@model[:name])
         rescue Puppet::ExecutionFailure
             return :false
         end
@@ -47,11 +47,11 @@ Puppet::Type.type(:service).provide :redhat, :parent => :init do
     # in the init scripts.
     def enable
         begin
-            output = util_execute("#{command(:chkconfig)} --add #{@model[:name]} 2>&1")
-            output += util_execute("#{command(:chkconfig)} #{@model[:name]} on 2>&1")
-        rescue Puppet::ExecutionFailure
+            output = chkconfig("--add", @model[:name])
+            output += chkconfig(@model[:name], :on)
+        rescue Puppet::ExecutionFailure => detail
             raise Puppet::Error, "Could not enable %s: %s" %
-                [self.name, output]
+                [self.name, detail]
         end
     end
 end
diff --git a/lib/puppet/provider/service/smf.rb b/lib/puppet/provider/service/smf.rb
index becd6fa3f..08f2bd731 100755
--- a/lib/puppet/provider/service/smf.rb
+++ b/lib/puppet/provider/service/smf.rb
@@ -27,11 +27,11 @@ Puppet::Type.type(:service).provide :smf, :parent => :base do
     end
 
     def restartcmd
-        "#{command(:adm)} restart %s" % @model[:name]
+        [command(:adm), :restart, @model[:name]]
     end
 
     def startcmd
-        "#{command(:adm)} enable %s" % @model[:name]
+        [command(:adm), :enable, @model[:name]]
     end
 
     def status
@@ -40,7 +40,7 @@ Puppet::Type.type(:service).provide :smf, :parent => :base do
             return
         end
         begin
-            output = Puppet::Util.execute("#{command(:svcs)} -l #{@model[:name]}")
+            output = svcs "-l", @model[:name]
         rescue Puppet::ExecutionFailure
             warning "Could not get status on service %s" % self.name
             return :stopped
@@ -77,7 +77,7 @@ Puppet::Type.type(:service).provide :smf, :parent => :base do
     end
 
     def stopcmd
-        "#{command(:adm)} disable %s" % @model[:name]
+        [command(:adm), :disable, @model[:name]]
     end
 end
 
diff --git a/lib/puppet/provider/user/pw.rb b/lib/puppet/provider/user/pw.rb
index 393e2d8e0..bc81e5ce5 100644
--- a/lib/puppet/provider/user/pw.rb
+++ b/lib/puppet/provider/user/pw.rb
@@ -26,7 +26,7 @@ Puppet::Type.type(:user).provide :pw, :parent => Puppet::Provider::NameService::
             # the value needs to be quoted, mostly because -c might
             # have spaces in it
             if value = @model.should(state) and value != ""
-                cmd << flag(state) << "'%s'" % value
+                cmd << flag(state) << value
             end
         end
 
@@ -34,7 +34,7 @@ Puppet::Type.type(:user).provide :pw, :parent => Puppet::Provider::NameService::
             cmd << "-o"
         end
 
-        return cmd.join(" ")
+        return cmd
     end
 end
 
diff --git a/lib/puppet/provider/user/useradd.rb b/lib/puppet/provider/user/useradd.rb
index 3b217e013..d8adb8c69 100644
--- a/lib/puppet/provider/user/useradd.rb
+++ b/lib/puppet/provider/user/useradd.rb
@@ -24,7 +24,7 @@ Puppet::Type.type(:user).provide :useradd, :parent => Puppet::Provider::NameServ
             # the value needs to be quoted, mostly because -c might
             # have spaces in it
             if value = @model.should(state) and value != ""
-                cmd << flag(state) << "'%s'" % value
+                cmd << flag(state) << value
             end
         end
         # stupid fedora
@@ -39,7 +39,7 @@ Puppet::Type.type(:user).provide :useradd, :parent => Puppet::Provider::NameServ
 
         cmd << @model[:name]
 
-        cmd.join(" ")
+        cmd
     end
 end
 
diff --git a/lib/puppet/reports/store.rb b/lib/puppet/reports/store.rb
index e3a3481ad..c8d7cdfce 100644
--- a/lib/puppet/reports/store.rb
+++ b/lib/puppet/reports/store.rb
@@ -1,15 +1,6 @@
 require 'puppet'
 
 Puppet::Server::Report.newreport(:store, :useyaml => true) do
-    Puppet.setdefaults(:reporting,
-        :reportdir => {:default => "$vardir/reports",
-                :mode => 0750,
-                :owner => "$user",
-                :group => "$group",
-                :desc => "The directory in which to store reports received from the
-            client.  Each client gets a separate subdirectory."}
-    )
-
     Puppet.config.use(:reporting)
 
     desc "Store the yaml report on disk.  Each host sends its report as a YAML dump
diff --git a/lib/puppet/server/report.rb b/lib/puppet/server/report.rb
index efebf1b4e..96cd818cb 100755
--- a/lib/puppet/server/report.rb
+++ b/lib/puppet/server/report.rb
@@ -28,7 +28,14 @@ class Server
                 "The list of reports to generate.  All reports are looked for
                 in puppet/reports/<name>.rb, and multiple report names should be
                 comma-separated (whitespace is okay)."
-            ]
+            ],
+            :reportdir => {:default => "$vardir/reports",
+                    :mode => 0750,
+                    :owner => "$user",
+                    :group => "$group",
+                    :desc => "The directory in which to store reports
+                        received from the client.  Each client gets a separate
+                        subdirectory."}
         )
 
         @reports = {}
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index 375571c3a..e4ace12e2 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -90,10 +90,14 @@ module Puppet
                     # have been instantiated.
                     if bucketobj = Puppet::Type.type(:filebucket)[value]
                         @parent.bucket = bucketobj.bucket
+                        bucketobj.title
                     else
+                        # Set it to the string; finish() turns it into a
+                        # filebucket.
                         @parent.bucket = value
+                        value
                     end
-                when Puppet::Client::Dipper: value
+                when Puppet::Client::Dipper: value.name
                 else
                     self.fail "Invalid backup type %s" %
                         value.inspect
@@ -264,23 +268,25 @@ module Puppet
         end
 
         # We have to do some extra finishing, to retrieve our bucket if
-        # there is one
+        # there is one.
         def finish
             # Let's cache these values, since there should really only be
             # a couple of these buckets
             @@filebuckets ||= {}
 
             # Look up our bucket, if there is one
-            if @parameters.include?(:backup) and bucket = self.bucket
+            if bucket = self.bucket
                 case bucket
                 when String:
                     if obj = @@filebuckets[bucket]
                         # This sets the @value on :backup, too
                         self.bucket = obj
-                    # elsif bucket == "puppet"
-                    #     obj = Puppet::Client::Dipper.new(
-                    #         :Path => Puppet[:puppetdir]
-                    #     )
+                    elsif bucket == "puppet"
+                        obj = Puppet::Client::Dipper.new(
+                            :Path => Puppet[:bucketdir]
+                        )
+                        self.bucket = obj
+                        @@filebuckets[bucket] = obj
                     elsif obj = Puppet::Type.type(:filebucket).bucket(bucket)
                         @@filebuckets[bucket] = obj
                         self.bucket = obj
@@ -320,7 +326,7 @@ module Puppet
                     # we don't need to backup directories when recurse is on
                     return true
                 else
-                    backup = self[:backup]
+                    backup = self.bucket || self[:backup]
                     case backup
                     when Puppet::Client::Dipper:
                         notice "Recursively backing up to filebucket"
@@ -359,7 +365,7 @@ module Puppet
                     end
                 end
             when "file":
-                backup = self[:backup]
+                backup = self.bucket || self[:backup]
                 case backup
                 when Puppet::Client::Dipper:
                     sum = backup.backup(file)
diff --git a/lib/puppet/type/zone.rb b/lib/puppet/type/zone.rb
index c882e199c..7576d173f 100644
--- a/lib/puppet/type/zone.rb
+++ b/lib/puppet/type/zone.rb
@@ -419,7 +419,7 @@ set zonepath=%s
 
     def destroy
         begin
-            execute("/usr/sbin/zonecfg -z #{self[:name]} delete -F")
+            execute(["/usr/sbin/zonecfg", "-z", self[:name], :delete, "-F"])
         rescue Puppet::ExecutionFailure => detail
             self.fail "Could not destroy zone: %s" % detail
         end
@@ -427,7 +427,7 @@ set zonepath=%s
 
     def install
         begin
-            execute("/usr/sbin/zoneadm -z #{self[:name]} install")
+            execute(["/usr/sbin/zoneadm", "-z", self[:name], :install])
         rescue Puppet::ExecutionFailure => detail
             self.fail "Could not install zone: %s" % detail
         end
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index bddcf8533..ef9f390f1 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -272,8 +272,14 @@ module Util
             command = command.collect { |i| i.to_s }
             str = command.join(" ")
         else
-            raise "Must pass an array" unless uid.nil? && gid.nil?
-            str = command
+            # We require an array here so we know where we're incorrectly
+            # using a string instead of an array.  Once everything is
+            # switched to an array, we might relax this requirement.
+            raise ArgumentError, "Must pass an array to execute()"
+        end
+
+        if command[0].is_a?(Array)
+            raise ArgumentError, "Will not flatten arrays"
         end
         if respond_to? :debug
             debug "Executing '%s'" % str