Changes based on code review.

src/lib/y2network/connection_config/qeth.rb

@@ -65,8 +65,8 @@ def initialize
       def ==(other)
         return false unless super
 
-        [:read_channel, :write_channel, :data_channel,
-         :layer2, :port_number, :attributes].all? do |method|
+        [:read_channel, :write_channel, :data_channel, :layer2,
+         :port_number, :ipa_takeover, :attributes].all? do |method|
           public_send(method) == other.public_send(method)
         end
       end

src/lib/y2network/dialogs/s390_ctc_activation.rb

@@ -24,6 +24,8 @@ module Dialogs
     # Dialog for activating a CTC device
     class S390CtcActivation < S390DeviceActivation
       def contents
+        # Already defined in the base class but added here just because of the
+        # pot check
         textdomain "network"
 
         HBox(

src/lib/y2network/dialogs/s390_device_activation.rb

@@ -61,7 +61,7 @@ def initialize(activator)
         textdomain "network"
 
         @activator = activator
-        @activator.proposal
+        @activator.propose!
         @builder = activator.builder
       end
 
@@ -79,7 +79,7 @@ def run
           configured = activator.configure
           builder.name = activator.configured_interface if configured
           # TODO: Refresh the list of interfaces in yast_config. Take into
-          # account that the interface in yast_config does not have a nem so
+          # account that the interface in yast_config does not have a name so
           # the builder.interface is probably nil and should be obtained
           # through the busid.
           if !configured || builder.name.empty?

src/lib/y2network/interfaces_collection.rb

@@ -74,7 +74,7 @@ def by_name(name)
     # @return [Interface,nil] Interface with the given busid or nil if not found
     def by_busid(busid)
       interfaces.find do |iface|
-        iface.hardware.busid == busid
+        iface.hardware && iface.hardware.busid == busid
       end
     end
 

src/lib/y2network/s390_device_activator.rb

@@ -26,6 +26,13 @@ class S390DeviceActivator
     extend Forwardable
     include Yast::Logger
 
+    # Command for configuring z Systems specific devices
+    CONFIGURE_CMD = "/sbin/chzdev".freeze
+    # Command for displaying configuration of z Systems specific devices
+    LIST_CMD = "/sbin/lszdev".freeze
+
+    def_delegators :@builder, :type
+
     attr_accessor :builder
 
     # Load fresh instace of device activator for given interface config builder
@@ -45,18 +52,23 @@ def initialize(builder)
       @builder = builder
     end
 
+    # Each s390 device type permits a set of attributes to be passed as extra
+    # options to the configuration command. This method return a list of each
+    # option in the form "attribute=value"
+    #
+    # @example qeth options
+    #   @activator.configure_attributes
+    #   #=> ["bridge_role=primary", "layer2=1", "portno=0", "ipa_takeover/enable=1"]
+    #
+    # @example ctc options
+    #   @activator.configure_attributes
+    #   #=> ["protocol=1"]
+    #
     # @return [Array<String>]
     def configure_attributes
       []
     end
 
-    # Convenience method for obtaining the short name of the builder's type..
-    #
-    # @return string
-    def type
-      builder.type.short_name
-    end
-
     # The device id to be used by lszdev or chzdev commands
     #
     # @return [String, nil]
@@ -69,7 +81,7 @@ def device_id
     # @param channel [String]
     # @return [String]
     def device_id_from(channel)
-      cmd = "/sbin/lszdev #{type} -c id -n".split(" ")
+      cmd = [LIST_CMD, type.short_name, "-c", "id", "-n"]
 
       Yast::Execute.stdout.on_target!(cmd).split("\n").find do |d|
         d.include? channel
@@ -81,7 +93,7 @@ def device_id_from(channel)
     # @return [Boolean] true when enabled
     def configure
       return false unless device_id
-      cmd = "/sbin/chzdev #{type} #{device_id} -e".split(" ").concat(configure_attributes)
+      cmd = [CONFIGURE_CMD, type.short_name, device_id, "-e"].concat(configure_attributes)
 
       Yast::Execute.on_target!(*cmd, allowed_exitstatus: 0..255).zero?
     end
@@ -91,13 +103,13 @@ def configure
     # @return [String] device name
     def configured_interface
       return "" unless device_id
-      cmd = "/sbin/lszdev #{device_id} -c names -n".split(" ")
+      cmd = [LIST_CMD, device_id, "-c", "names", "-n"]
 
       Yast::Execute.stdout.on_target!(cmd).chomp
     end
 
     # Makes a new configuration proposal
-    def proposal
+    def propose!
     end
   end
 end

src/lib/y2network/s390_device_activators/ctc.rb

@@ -47,7 +47,7 @@ def propose_channels
         self.read_channel, self.write_channel = id.split(":")
       end
 
-      def proposal
+      def propose!
         propose_channels unless device_id
       end
     end

src/lib/y2network/s390_device_activators/lcs.rb

@@ -24,7 +24,7 @@ module S390DeviceActivators
     # The Lcs device activator is based in Ctc as both have two group device
     # channels (read and write).
     #
-    # In the past they shared also the configure command 'ctc_configure)' and
+    # In the past they shared also the configure command 'ctc_configure' and
     # the 'protocol' attribute was needed, but as the configuration has
     # been moved to 'chzdev' command it is not the case anymore.
     class Lcs < Ctc

src/lib/y2network/s390_device_activators/qeth.rb

@@ -28,7 +28,7 @@ class Qeth < S390DeviceActivator
         :read_channel, :read_channel=,
         :write_channel, :write_channel=,
         :data_channel, :data_channel=,
-        :layer2, :port_number,
+        :layer2, :port_number, :ipa_takeover,
         :hwinfo, :attributes
 
       def device_id
@@ -37,11 +37,15 @@ def device_id
         [read_channel, write_channel, data_channel].join(":")
       end
 
-      # @return [Array<String>]
+      # Return a list of extra attributes to be set when activating the device
+      #
+      # @see [S390DeviceActivator#configure_attributes]
       def configure_attributes
-        return [] unless attributes
-
-        attributes.split(" ").concat([layer2_attribute, port_ttribute])
+        extra_attributes = []
+        extra_attributes.concat(attributes.split(" ")) if attributes
+        extra_attributes << ipa_takeover_attribute unless ipa_takeover.nil?
+        extra_attributes << layer2_attribute unless layer2.nil?
+        extra_attributes << port_attribute
       end
 
       # Modifies the read, write and data channel from the the device id
@@ -51,27 +55,35 @@ def propose_channels
         self.read_channel, self.write_channel, self.data_channel = id.split(":")
       end
 
-      def proposal
+      def propose!
         propose_channels unless device_id
       end
-    end
 
-  private
+    private
 
-    # Convenience method to obtain the layer2 attribute
-    #
-    # @return [String]
-    def layer2_attribute
-      return "" unless layer2
+      # Convenience method to obtain the layer2 attribute for the configuration
+      # command
+      #
+      # @return [String]
+      def layer2_attribute
+        "layer2=#{layer2 ? 1 : 0}"
+      end
 
-      "layer2=#{layer2 ? 1 : 0}"
-    end
+      # Convenience method to obtain the port number attribute for the
+      # configuration command
+      #
+      # @return [String]
+      def port_attribute
+        "portno=#{port_number}"
+      end
 
-    # Convenience method to obtain the port number attribute
-    #
-    # @return [String]
-    def port_attribute
-      "port_number=#{port_number}"
+      # Convenience method to obtain the port number attribute for the
+      # configuration command
+      #
+      # @return [String]
+      def ipa_takeover_attribute
+        "ipa_takeover/enable=#{ipa_takeover ? 1 : 0}"
+      end
     end
   end
 end

src/lib/y2network/sysconfig/config_writer.rb

@@ -67,6 +67,8 @@ def write(config, old_config = nil)
       def write_interface_changes(config, old_config)
         # Write ifroute files
         config.interfaces.each do |dev|
+          # S390 devices that have not been activated yet will be part of the
+          # collection but with an empty name.
           next if dev.name.empty?
           routes = find_routes_for(dev, config.routing.routes)
           file = routes_file_for(dev)

test/y2network/dialogs/s390_device_activation_test.rb

@@ -33,7 +33,7 @@
 
   describe ".new" do
     it "creates a proposal for the configured device" do
-      expect(activator).to receive(:proposal)
+      expect(activator).to receive(:propose!)
       described_class.new(activator)
     end
   end

test/y2network/s390_device_activators/ctc_test.rb

@@ -120,19 +120,19 @@
     end
   end
 
-  describe "#proposal" do
+  describe "#propose!" do
     context "when no device id has been initialized" do
       let(:initialize_channels) { false }
       it "proposes the channel device ids to be used" do
         expect(subject).to receive(:propose_channels)
-        subject.proposal
+        subject.propose!
       end
     end
 
     context "when the channel device ids have been set already" do
       it "does not propose anything" do
         expect(subject).to_not receive(:propose_channels)
-        subject.proposal
+        subject.propose!
       end
     end
   end

test/y2network/s390_device_activators/qeth_test.rb

@@ -47,7 +47,9 @@
   describe "#configure" do
     it "tries to activate the group device associated with the defined device id" do
       expect(Yast::Execute).to receive(:on_target!)
-        .with("/sbin/chzdev", "qeth", subject.device_id, "-e", allowed_exitstatus: 0..255)
+        .with("/sbin/chzdev", "qeth", subject.device_id, "-e",
+          "ipa_takeover/enable=0", "layer2=0", "portno=0",
+          allowed_exitstatus: 0..255)
         .and_return(0)
       subject.configure
     end
@@ -121,19 +123,19 @@
     end
   end
 
-  describe "#proposal" do
+  describe "#propose!" do
     context "when no device id has been initialized" do
       let(:initialize_channels) { false }
       it "proposes the channel device ids to be used" do
         expect(subject).to receive(:propose_channels)
-        subject.proposal
+        subject.propose!
       end
     end
 
     context "when the channel device ids have been set already" do
       it "does not propose anything" do
         expect(subject).to_not receive(:propose_channels)
-        subject.proposal
+        subject.propose!
       end
     end
   end