This further normalizes the handling of init-style services (including

the redhat "service" wrapper script).  Removes special case handling of
non-zero exit code in redhat (base already did this) and centralizes
scattered @resource[:has_____] checks.  Tests that proper versions of
each are called and one level of fallbacks.

Signed-off-by: Markus Roberts <Markus@reality.com>

3 files changed, 36 insertions, 58 deletions

diff --git a/lib/puppet/provider/service/base.rb b/lib/puppet/provider/service/base.rb
index 462dd897d..0bf2b20ea 100755
--- a/lib/puppet/provider/service/base.rb
+++ b/lib/puppet/provider/service/base.rb
@@ -19,13 +19,11 @@ Puppet::Type.type(:service).provide :base do
     # parameter.
     def getpid
         unless @resource[:pattern]
-            @resource.fail "Either a stop command or a pattern must be specified"
+            @resource.fail "Either stop/status commands or a pattern must be specified"
         end
         ps = Facter["ps"].value
         unless ps and ps != ""
-            @resource.fail(
-                "You must upgrade Facter to a version that includes 'ps'"
-            )
+            @resource.fail "You must upgrade Facter to a version that includes 'ps'"
         end
         regex = Regexp.new(@resource[:pattern])
         self.debug "Executing '#{ps}'"
@@ -43,7 +41,7 @@ Puppet::Type.type(:service).provide :base do
 
     # How to restart the process.
     def restart
-        if @resource[:restart] or self.respond_to?(:restartcmd)
+        if @resource[:restart] or restartcmd
             ucommand(:restart)
         else
             self.stop
@@ -51,19 +49,22 @@ Puppet::Type.type(:service).provide :base do
         end
     end
 
+    # There is no default command, which causes other methods to be used  
+    def restartcmd
+    end
+
     # Check if the process is running.  Prefer the 'status' parameter,
     # then 'statuscmd' method, then look in the process table.  We give
     # the object the option to not return a status command, which might
     # happen if, for instance, it has an init script (and thus responds to
     # 'statuscmd') but does not have 'hasstatus' enabled.
     def status
-        if @resource[:status] or (
-            self.respond_to?(:statuscmd) and self.statuscmd
-        )
+        if @resource[:status] or statuscmd
             # Don't fail when the exit status is not 0.
-            output = ucommand(:status, false)
+            ucommand(:status, false)
 
-            if $? == 0
+            # Expicitly calling exitstatus to facilitate testing
+            if $?.exitstatus == 0
                 return :running
             else
                 return :stopped
@@ -76,6 +77,10 @@ Puppet::Type.type(:service).provide :base do
         end
     end
 
+    # There is no default command, which causes other methods to be used  
+    def statuscmd
+    end
+    
     # Run the 'start' parameter command, or the specified 'startcmd'.
     def start
         ucommand(:start)
@@ -98,7 +103,7 @@ Puppet::Type.type(:service).provide :base do
     # for the process in the process table.
     # This method will generally not be overridden by submodules.
     def stop
-        if @resource[:stop] or self.respond_to?(:stopcmd)
+        if @resource[:stop] or stopcmd
             ucommand(:stop)
         else
             pid = getpid
@@ -115,6 +120,10 @@ Puppet::Type.type(:service).provide :base do
             return true
         end
     end
+    
+    # There is no default command, which causes other methods to be used  
+    def stopcmd
+    end
 
     # A simple wrapper so execution failures are a bit more informative.
     def texecute(type, command, fof = true)
diff --git a/lib/puppet/provider/service/init.rb b/lib/puppet/provider/service/init.rb
index 1bf11fbba..965773d96 100755
--- a/lib/puppet/provider/service/init.rb
+++ b/lib/puppet/provider/service/init.rb
@@ -67,20 +67,7 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
 
     # Where is our init script?
     def initscript
-        if defined? @initscript
-            return @initscript
-        else
-            @initscript = self.search(@resource[:name])
-        end
-    end
-
-    def restart
-        if @resource[:hasrestart] == :true
-            command = [self.initscript, :restart]
-            texecute("restart", command)
-        else
-            super
-        end
+        @initscript ||= self.search(@resource[:name])
     end
 
     def search(name)
@@ -115,23 +102,24 @@ Puppet::Type.type(:service).provide :init, :parent => :base do
 
     # The start command is just the init scriptwith 'start'.
     def startcmd
-        [self.initscript, :start]
+        [initscript, :start]
+    end
+
+    # The stop command is just the init script with 'stop'.
+    def stopcmd
+        [initscript, :stop]
+    end
+
+    def restartcmd
+        (@resource[:hasrestart] == :true) && [initscript, :restart]
     end
 
     # If it was specified that the init script has a 'status' command, then
     # we just return that; otherwise, we return false, which causes it to
     # fallback to other mechanisms.
     def statuscmd
-        if @resource[:hasstatus] == :true
-            return [self.initscript, :status]
-        else
-            return false
-        end
+        (@resource[:hasstatus] == :true) && [initscript, :status]
     end
 
-    # The stop command is just the init script with 'stop'.
-    def stopcmd
-        [self.initscript, :stop]
-    end
 end
 
diff --git a/lib/puppet/provider/service/redhat.rb b/lib/puppet/provider/service/redhat.rb
index 8c12782d5..211b66956 100755
--- a/lib/puppet/provider/service/redhat.rb
+++ b/lib/puppet/provider/service/redhat.rb
@@ -52,35 +52,16 @@ Puppet::Type.type(:service).provide :redhat, :parent => :init do
         end
     end
 
-    def restart
-        if @resource[:hasrestart] == :true
-            service(@resource[:name], "restart")
-        else
-            super
-        end
-    end
-
-    def status
-        if @resource[:status]
-            super
-        elsif @resource[:hasstatus] == :true
-            begin
-                service(@resource[:name], "status")
-                return :running
-            rescue
-                return :stopped
-            end
-        else
-            super
-        end
+    def initscript
+        raise Puppet::Error, "Do not directly call the init script for '%s'; use 'service' instead" % @resource[:name]
     end
 
     def statuscmd
-        [command(:service), @resource[:name], "status"]
+        (@resource[:hasstatus] == :true) && [command(:service), @resource[:name], "status"]
     end
 
     def restartcmd
-        [command(:service), @resource[:name], "restart"]
+        (@resource[:hasrestart] == :true) && [command(:service), @resource[:name], "restart"]
     end
 
     def startcmd