Added unit test and refactorized code

src/modules/NetHwDetection.rb

@@ -55,6 +55,7 @@
 # </ul>
 #
 require "yast"
+require "yast2/execute"
 require "shellwords"
 
 module Yast
@@ -85,47 +86,17 @@ def main
     # WATCH OUT, this is the place where modules are loaded
     # @return true if success
     def LoadNetModules
-      Builtins.y2milestone("Network detection prepare")
+      log.info "Network detection prepare"
 
       hardware = ReadHardware("netcard")
 
-      Builtins.y2debug("Hardware=%1", hardware)
-      return false if Ops.less_than(Builtins.size(hardware), 1)
-
-      needed_modules = Builtins.listmap(hardware) do |h|
-        # Formerly we simply modprobed the first module of the first
-        # driver, if it was not already loaded.  But if the user
-        # configured the card to use the second driver and unloads it
-        # and wants to load the first, it will not work because the
-        # first driver is already loaded but not bound to the device
-        # (the second one took it). N#59794#c31
-        # We will only load a driver if there's no driver for the card active.
-        active_driver = Builtins.find(Ops.get_list(h, "drivers", [])) do |d|
-          Ops.get_boolean(d, "active", false)
-        end
-        { Ops.get_string(h, "module", "") => active_driver.nil? }
-        # TODO: list of todos
-        # 1: choose which driver to load
-        # 2: load all its modules: no cards use multiple modules
-        # 3: either modprobe or insmod: ISA history
-      end
-      needed_modules = Builtins.filter(needed_modules) do |m, load|
-        load && !m.nil? && m != "" &&
-          SCR.Execute(
-            path(".target.bash"),
-            Builtins.sformat("/usr/bin/grep ^%1 /proc/modules", m.shellescape)
-          ) != 0
-      end
-      @detection_modules = Builtins.maplist(needed_modules) { |m, _a| m }
-      Builtins.foreach(@detection_modules) do |mod|
-        Builtins.y2milestone("Loading module: %1", mod)
-        SCR.Execute(
-          path(".target.bash"),
-          Builtins.sformat("/usr/sbin/modprobe --use-blacklist %1 2>&1", mod.shellescape)
-        )
-      end
+      log.debug("Hardware=#{hardware.inspect}")
+      return false if hardware.empty?
+
+      @detection_modules = needed_modules(hardware).dup
+      @detection_modules.each { |name| load_module(name) }
 
-      Builtins.y2milestone("Network detection prepare (end)")
+      log.info("Network detection prepare (end)")
 
       true
     end
@@ -239,6 +210,53 @@ def ResolveIP(ip)
     publish function: :Stop, type: "boolean ()"
     publish function: :DuplicateIP, type: "boolean (string)"
     publish function: :ResolveIP, type: "string (string)"
+
+  private
+
+    # Check which modules need to be modprobed returning the ones which are not
+    # active according to hwinfo and which are still not loaded
+    #
+    # @param netcards_hwinfo [Array<Hwinfo>] network devices info
+    # @return [Array<String>]
+    def needed_modules(netcards_hwinfo)
+      netcards_hwinfo.each_with_object([]) do |h, modules|
+        name = h.fetch("module", "")
+        next if name.empty?
+
+        modules << name if !active_driver?(h) && !already_loaded?(name)
+      end
+    end
+
+    # Convenience method to check whether a given module is already loaded or
+    # not
+    #
+    # @param name [String] module name
+    # @return [Boolean] whether a given module is already loaded or not
+    def already_loaded?(name)
+      cmd = ["/usr/bin/grep", "^#{name}", "/proc/modules"]
+
+      _output, status = Yast::Execute.stdout.on_target(*cmd, allowed_exitstatus: 0..255)
+      status&.zero?
+    end
+
+    # Convenience method to check wheter a driver is already active or not
+    # according to the given hwinfo
+    #
+    # @param netcard_hwinfo [Array<Hash>] netcard hardware info
+    # @return [Boolean] whether the netcard driver is active or not
+    def active_driver?(netcard_hwinfo)
+      netcard_hwinfo.fetch("drivers", []).any? { |d| d["active"] }
+    end
+
+    # Convenience method to modprobe the given module
+    #
+    # @param name [String] module to be modprobed
+    def load_module(name)
+      cmd = ["/usr/sbin/modprobe", "--use-blacklist", name]
+
+      log.info("Loading module: #{name}")
+      Yast::Execute.stdout.on_target(*cmd, allowed_exitstatus: 0..1)
+    end
   end
 
   NetHwDetection = NetHwDetectionClass.new

test/data/scr_root/proc/modules

@@ -0,0 +1,4 @@
+qeth_l2 61440 1 - Live 0x000003ff806d0000
+qeth 147456 1 qeth_l2, Live 0x000003ff801b3000
+qdio 73728 3 zfcp,qeth_l2,qeth, Live 0x000003ff8019a000
+ccwgroup 20480 3 lcs,ctcm,qeth, Live 0x000003ff80190000

test/net_hw_detection_test.rb

@@ -0,0 +1,96 @@
+#!/usr/bin/env rspec
+
+# Copyright (c) [2020] SUSE LLC
+#
+# All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of version 2 of the GNU General Public License as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, contact SUSE LLC.
+#
+# To contact SUSE LLC about this file by physical or electronic mail, you may
+# find current contact information at www.suse.com.
+
+require_relative "test_helper"
+
+Yast.import "NetHwDetection"
+
+describe "NetHwDetection" do
+  subject { Yast::NetHwDetection }
+  let(:hwinfo) { [] }
+
+  before do
+    allow(subject).to receive(:ReadHardware).with("netcard").and_return(hwinfo)
+  end
+
+  describe "#LoadNetModules" do
+    it "reads the hwinfo of the network cards present in the system" do
+      expect(subject).to receive(:ReadHardware).with("netcard")
+      subject.LoadNetModules
+    end
+
+    context "when the system does not have network cards" do
+      it "returns false" do
+        expect(subject.LoadNetModules).to eq(false)
+      end
+    end
+
+    context "when there is some network card which driver is not active" do
+      let(:loaded) { true }
+
+      before do
+        allow(subject).to receive(:already_loaded?).with("qeth").and_return(loaded)
+      end
+
+      let(:hwinfo) do
+        [
+          {
+            "active" => false, "bus" => "none", "busid" => "", "dev_name" => "", "driver" => "",
+            "drivers" => [{ "active" => false, "modprobe" => true, "modules" => [["qeth", ""]] }],
+            "link" => nil, "mac" => "", "modalias" => "", "module" => "qeth",
+            "name" => "OSA Express Network card", "num" => 0, "options" => "",
+            "permanent_mac" => "", "requires" => [], "sysfs_id" => "", "type" => "qeth",
+            "udi" => "", "unique" => "rdCR.n_7QNeEnh23", "wl_auth_modes" => nil,
+            "wl_bitrates" => nil, "wl_channels" => nil, "wl_enc_modes" => nil
+          },
+          {
+            "active" => true, "bus" => "Virtio", "busid" => "virtio0", "dev_name" => "eth0",
+            "drivers" => [
+              { "active" => true, "modprobe" => true, "modules" => [["virtio_net", ""]] }
+            ], "driver" => "virtio_net", "link" => false, "mac" => "52:54:00:12:34:56",
+            "modalias" => "virtio:d00000001v00001AF4", "module" => "virtio_net",
+            "name" => "Ethernet Card 0", "num" => 1, "options" => "",
+            "parent_busid" => "0000:00:02.0", "permanent_mac" => "52:54:00:12:34:56",
+            "requires" => [], "sysfs_id" => "/devices/pci0000:00/0000:00 =>02.0/virtio0",
+            "type" => "eth", "udi" => "", "unique" => "Prmq.VIRhsc57kTD", "wl_auth_modes" => nil,
+            "wl_bitrates" => nil, "wl_channels" => nil, "wl_enc_modes" => nil
+          }
+        ]
+      end
+
+      context "and the driver module is not loaded" do
+        let(:loaded) { false }
+
+        it "modprobes the inactive driver" do
+          expect(subject).to receive(:load_module).with("qeth")
+          subject.LoadNetModules
+        end
+      end
+
+      context "and the driver was already loaded" do
+        it "does not try to modprobe the module" do
+          expect(subject).to_not receive(:load_module)
+          subject.LoadNetModules
+        end
+      end
+    end
+  end
+end