review feedback plus break out helper class

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

4 files changed, 136 insertions, 100 deletions

diff --git a/lib/chef/provider/package/dnf.rb b/lib/chef/provider/package/dnf.rb
index c22f06f52c..0d026a49b7 100644
--- a/lib/chef/provider/package/dnf.rb
+++ b/lib/chef/provider/package/dnf.rb
@@ -1,3 +1,4 @@
+#
 # Copyright:: Copyright 2016, Chef Software, Inc.
 # License:: Apache License, Version 2.0
 #
@@ -18,7 +19,7 @@ require "chef/provider/package"
 require "chef/resource/dnf_package"
 require "chef/mixin/which"
 require "chef/mixin/get_source_from_package"
-require "timeout"
+require "chef/provider/package/dnf/python_helper"
 
 class Chef
   class Provider
@@ -27,17 +28,16 @@ class Chef
         extend Chef::Mixin::Which
         include Chef::Mixin::GetSourceFromPackage
 
-        allow_nils
-
+        # helper class to assist in passing around name/version/arch triples
         class Version
           attr_accessor :name
           attr_accessor :version
           attr_accessor :arch
 
           def initialize(name, version, arch)
-            @name = name
-            @version = ( version == "nil" ) ? nil : version
-            @arch = ( arch == "nil" ) ? nil : arch
+            @name    = name
+            @version = version
+            @arch    = arch
           end
 
           def to_s
@@ -59,96 +59,7 @@ class Chef
           alias_method :eql?, :==
         end
 
-        attr_accessor :python_helper
-
-        class PythonHelper
-          include Singleton
-          extend Chef::Mixin::Which
-
-          attr_accessor :stdin
-          attr_accessor :stdout
-          attr_accessor :stderr
-          attr_accessor :wait_thr
-
-          DNF_HELPER = ::File.expand_path(::File.join(::File.dirname(__FILE__), "dnf_helper.py")).freeze
-          DNF_COMMAND = "#{which("python3")} #{DNF_HELPER}"
-
-          def start
-            ENV["PYTHONUNBUFFERED"] = "1"
-            @stdin, @stdout, @stderr, @wait_thr = Open3.popen3(DNF_COMMAND)
-          end
-
-          def reap
-            unless wait_thr.nil?
-              Process.kill("KILL", wait_thr.pid) rescue nil
-              stdin.close unless stdin.nil?
-              stdout.close unless stdout.nil?
-              stderr.close unless stderr.nil?
-              wait_thr.value
-            end
-          end
-
-          def check
-            start if stdin.nil?
-          end
-
-          # i couldn't figure out how to decompose an evr on the python side, it seems reasonably
-          # painless to do it in ruby.
-          def add_version(hash, version)
-            epoch = nil
-            if version =~ /(\S+):(\S+)/
-              epoch, version = $1, $2
-            end
-            if version =~ /(\S+)-(\S+)/
-              version, release = $1, $2
-            end
-            hash["epoch"] = epoch unless epoch.nil?
-            hash["release"] = release unless release.nil?
-            hash["version"] = version
-          end
-
-          # @returns Array<Version>
-          def query(action, provides, version = nil, arch = nil)
-            with_helper do
-              hash = { "action" => action }
-              hash["provides"] = provides
-              add_version(hash, version) unless version.nil?
-              hash["arch" ] = arch unless arch.nil?
-              json = FFI_Yajl::Encoder.encode(hash)
-              puts json
-              stdin.syswrite json + "\n"
-              output = stdout.sysread(4096)
-              puts output
-              output.split.each_slice(3).map { |x| Version.new(*x) }.first
-            end
-          end
-
-          def flushcache
-            restart # FIXME: make flushcache work + not leak memory
-          end
-
-          def flushcache_installed
-            restart # FIXME: make flushcache work + not leak memory
-          end
-
-          def restart
-            reap
-            start
-          end
-
-          def with_helper
-            max_retries ||= 5
-            Timeout.timeout(60) do
-              check
-              yield
-            end
-          rescue EOFError, Errno::EPIPE, Timeout::Error, Errno::ESRCH => e
-            raise e unless ( max_retries -= 1 ) > 0
-            restart
-            retry
-          end
-        end
-
+        allow_nils
         use_multipackage_api
         use_package_name_for_source
 
diff --git a/lib/chef/provider/package/dnf_helper.py b/lib/chef/provider/package/dnf/dnf_helper.py
index d9f753eeec..19262c2ec8 100644
--- a/lib/chef/provider/package/dnf_helper.py
+++ b/lib/chef/provider/package/dnf/dnf_helper.py
@@ -8,8 +8,6 @@ import signal
 import os
 import json
 
-from pprint import pprint
-
 base = None
 
 def get_sack():
@@ -63,6 +61,9 @@ def query(command):
         pkg = pkgs.pop(0)
         sys.stdout.write('{} {}:{}-{} {}\n'.format(pkg.name, pkg.epoch, pkg.version, pkg.release, pkg.arch))
 
+# the design of this helper is that it should try to be 'brittle' and fail hard and exit in order
+# to keep process tables clean.  additional error handling should probably be added to the retry loop
+# on the ruby side.
 def exit_handler(signal, frame):
     sys.exit(0)
 
@@ -72,6 +73,7 @@ signal.signal(signal.SIGPIPE, exit_handler)
 signal.signal(signal.SIGCHLD, exit_handler)
 
 while 1:
+    # kill self if we get orphaned (tragic)
     ppid = os.getppid()
     if ppid == 1:
         sys.exit(0)
diff --git a/lib/chef/provider/package/dnf/python_helper.rb b/lib/chef/provider/package/dnf/python_helper.rb
new file mode 100644
index 0000000000..b2538ef5b9
--- /dev/null
+++ b/lib/chef/provider/package/dnf/python_helper.rb
@@ -0,0 +1,124 @@
+#
+# Copyright:: Copyright 2016, Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require "timeout"
+
+class Chef
+  class Provider
+    class Package
+      class Dnf < Chef::Provider::Package
+        class PythonHelper
+          include Singleton
+          extend Chef::Mixin::Which
+
+          attr_accessor :stdin
+          attr_accessor :stdout
+          attr_accessor :stderr
+          attr_accessor :wait_thr
+
+          DNF_HELPER = ::File.expand_path(::File.join(::File.dirname(__FILE__), "dnf_helper.py")).freeze
+          DNF_COMMAND = "#{which("python3")} #{DNF_HELPER}"
+
+          def start
+            ENV["PYTHONUNBUFFERED"] = "1"
+            @stdin, @stdout, @stderr, @wait_thr = Open3.popen3(DNF_COMMAND)
+          end
+
+          def reap
+            unless wait_thr.nil?
+              Process.kill("KILL", wait_thr.pid) rescue nil
+              stdin.close unless stdin.nil?
+              stdout.close unless stdout.nil?
+              stderr.close unless stderr.nil?
+              wait_thr.value # this calls waitpit()
+            end
+          end
+
+          def check
+            start if stdin.nil?
+          end
+
+          # i couldn't figure out how to decompose an evr on the python side, it seems reasonably
+          # painless to do it in ruby (generally massaging nevras in the ruby side is HIGHLY
+          # discouraged -- this is an "every rule has an exception" exception -- any additional
+          # functionality should probably trigger moving this regexp logic into python)
+          def add_version(hash, version)
+            epoch = nil
+            if version =~ /(\S+):(\S+)/
+              epoch, version = $1, $2
+            end
+            if version =~ /(\S+)-(\S+)/
+              version, release = $1, $2
+            end
+            hash["epoch"] = epoch unless epoch.nil?
+            hash["release"] = release unless release.nil?
+            hash["version"] = version
+          end
+
+          def build_query(action, provides, version, arch)
+            hash = { "action" => action }
+            hash["provides"] = provides
+            add_version(hash, version) unless version.nil?
+            hash["arch" ] = arch unless arch.nil?
+            FFI_Yajl::Encoder.encode(hash)
+          end
+
+          def parse_response(output)
+            array = output.split.map { |x| x == "nil" ? nil : x }
+            array.each_slice(3).map { |x| Version.new(*x) }.first
+          end
+
+          # @returns Array<Version>
+          def query(action, provides, version = nil, arch = nil)
+            with_helper do
+              json = build_query(action, provides, version, arch)
+              stdin.syswrite json + "\n"
+              output = stdout.sysread(4096)
+              version = parse_response(output)
+              version
+            end
+          end
+
+          def flushcache
+            restart # FIXME: make flushcache work + not leak memory
+          end
+
+          def flushcache_installed
+            restart # FIXME: make flushcache work + not leak memory
+          end
+
+          def restart
+            reap
+            start
+          end
+
+          def with_helper
+            max_retries ||= 5
+            Timeout.timeout(600) do
+              check
+              yield
+            end
+          rescue EOFError, Errno::EPIPE, Timeout::Error, Errno::ESRCH => e
+            raise e unless ( max_retries -= 1 ) > 0
+            restart
+            retry
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/chef/resource/dnf_package.rb b/lib/chef/resource/dnf_package.rb
index 49bf44bd8b..92f7532fc2 100644
--- a/lib/chef/resource/dnf_package.rb
+++ b/lib/chef/resource/dnf_package.rb
@@ -1,6 +1,5 @@
 #
-# Author:: AJ Christensen (<aj@chef.io>)
-# Copyright:: Copyright 2008-2016, Chef Software, Inc.
+# Copyright:: Copyright 2016, Chef Software, Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");