Merge 9618429 into c786514

package/yast2-network.changes

@@ -1,3 +1,14 @@
+-------------------------------------------------------------------
+Thu Jun 12 11:55:48 UTC 2019 - Knut Anderssen <kanderssen@suse.com>
+
+- bsc#1136929
+  - Use the hwinfo permanent mac address instead of the current mac
+    which could be wrong in case of an enslaved interface.
+- bsc#1137324
+  - Do not create duplicate udev rule attributes when editing an
+    interface name.
+- 4.1.48
+
 -------------------------------------------------------------------
 Tue Jun 11 07:59:52 UTC 2019 - Knut Anderssen <kanderssen@suse.com>
 

package/yast2-network.spec

@@ -17,7 +17,7 @@
 
 
 Name:           yast2-network
-Version:        4.1.47
+Version:        4.1.48
 Release:        0
 BuildArch:      noarch
 

src/include/network/lan/complex.rb

@@ -308,8 +308,9 @@ def UpdateSlaves
         case LanItems.GetDeviceType(current)
         when "bond"
           LanItems.startmode = "hotplug"
-
-          LanItems.update_item_udev_rule!(:bus_id)
+          if LanItems.current_udev_rule.empty? || LanItems.GetItemUdev("KERNELS").empty?
+            LanItems.update_item_udev_rule!(:bus_id)
+          end
         when "br"
           LanItems.ipaddr = ""
         end

src/include/network/routines.rb

@@ -632,6 +632,7 @@ def ReadHardware(hwtype)
             one["parent_busid"] = one["sysfs_id"].split("/")[-2]
           end
           one["mac"] = Ops.get_string(resource, ["hwaddr", 0, "addr"], "")
+          one["permanent_mac"] = Ops.get_string(resource, ["phwaddr", 0, "addr"], "")
           # is the cable plugged in? nil = don't know
           one["link"] = Ops.get(resource, ["link", 0, "state"])
 

src/lib/network/edit_nic_name.rb

@@ -16,9 +16,9 @@ class EditNicName
     include I18n
 
     # @return [String] current udev name before modifying it
-    attr_accessor :old_name
+    attr_reader :old_name
     # @return [String] current udev match criteria
-    attr_accessor :old_key
+    attr_reader :old_key
 
     # udev rule attribute for MAC address
     MAC_UDEV_ATTR   = "ATTR{address}".freeze
@@ -32,13 +32,16 @@ def initialize
       Yast.include self, "network/routines.rb"
 
       current_item = LanItems.getCurrentItem
+      current_rule = LanItems.current_udev_rule
 
       @old_name = LanItems.current_udev_name
-      @old_key = MAC_UDEV_ATTR unless LanItems.GetItemUdev(MAC_UDEV_ATTR).empty?
-      @old_key = BUSID_UDEV_ATTR unless LanItems.GetItemUdev(BUSID_UDEV_ATTR).empty?
+      unless current_rule.empty?
+        @old_key = :mac unless LanItems.GetItemUdev(MAC_UDEV_ATTR).empty?
+        @old_key = :bus_id unless LanItems.GetItemUdev(BUSID_UDEV_ATTR).empty?
+      end
 
       if current_item["hwinfo"]
-        @mac = current_item["hwinfo"]["mac"]
+        @mac = current_item["hwinfo"]["permanent_mac"]
         @bus_id = current_item["hwinfo"]["busid"]
       else
         @mac = ""
@@ -73,7 +76,7 @@ def run
 
         # FIXME: it changes udev key used for device identification
         #  and / or its value only, name is changed elsewhere
-        LanItems.update_item_udev_rule!(udev_type)
+        LanItems.update_item_udev_rule!(udev_type) if udev_type && (old_key != udev_type)
 
         if new_name != old_name
           LanItems.update_routing_devices!
@@ -129,11 +132,8 @@ def open
         )
       )
 
-      case old_key
-      when MAC_UDEV_ATTR
-        UI.ChangeWidget(Id(:udev_type), :CurrentButton, :mac)
-      when BUSID_UDEV_ATTR
-        UI.ChangeWidget(Id(:udev_type), :CurrentButton, :bus_id)
+      if old_key
+        UI.ChangeWidget(Id(:udev_type), :CurrentButton, old_key)
       else
         Builtins.y2error("Unknown udev rule.")
       end

src/lib/network/network_autoyast.rb

@@ -339,7 +339,7 @@ def assign_udevs_to_devs(udev_rules)
           busid = i["hwinfo"]["busid"]
           # Match also parent busid if exist (bsc#1129012)
           parent_busid = i["hwinfo"]["parent_busid"] || busid
-          mac = i["hwinfo"]["mac"]
+          mac = i["hwinfo"]["permanent_mac"]
 
           [busid, parent_busid, mac].any? { |v| v.casecmp(key).zero? }
         end

src/modules/LanItems.rb

@@ -259,6 +259,13 @@ def GetDeviceName(item_id)
       "" # this should never happen
     end
 
+    # Convenience method to obtain the current Item udev rule
+    #
+    # @return [Array<String>] Item udev rule
+    def current_udev_rule
+      LanItems.GetItemUdevRule(LanItems.current)
+    end
+
     # Returns name which is going to be used in the udev rule
     def current_udev_name
       if LanItems.current_renamed?
@@ -370,7 +377,7 @@ def InitItemUdevRule(item_id)
       udev = GetItemUdevRule(item_id)
       return udev if !udev.empty?
 
-      default_mac = GetLanItem(item_id).fetch("hwinfo", {})["mac"]
+      default_mac = GetLanItem(item_id).fetch("hwinfo", {})["permanent_mac"]
       raise ArgumentError, "Cannot propose udev rule - NIC not present" if !default_mac
 
       default_udev = GetDefaultUdevRule(
@@ -430,7 +437,7 @@ def getUdevFallback
       if IsEmpty(udev_rules)
         udev_rules = GetDefaultUdevRule(
           GetCurrentName(),
-          Ops.get_string(getCurrentItem, ["hwinfo", "mac"], "")
+          Ops.get_string(getCurrentItem, ["hwinfo", "permanent_mac"], "")
         )
         Builtins.y2milestone(
           "No Udev rules found, creating default: %1",
@@ -452,13 +459,11 @@ def GetItemUdev(key)
     # @return [Object, nil] the current item's udev rule without the given key; nil if
     # there is not udev rules for the current item
     def RemoveItemUdev(key)
-      current_rule = LanItems.GetItemUdevRule(LanItems.current)
+      return nil if current_udev_rule.empty?
 
-      return nil if current_rule.empty?
-
-      log.info("Removing #{key} from #{current_rule}")
+      log.info("Removing #{key} from #{current_udev_rule}")
       Items()[@current]["udev"]["net"] =
-        LanItems.RemoveKeyFromUdevRule(current_rule, key)
+        LanItems.RemoveKeyFromUdevRule(current_udev_rule, key)
     end
 
     # Updates the udev rule of the current Lan Item based on the key given
@@ -471,8 +476,13 @@ def RemoveItemUdev(key)
     # @param based_on [Symbol] principal key to be matched, `:mac` or `:bus_id`
     # @return [void]
     def update_item_udev_rule!(based_on = :mac)
+      new_rule = current_udev_rule.empty?
+      LanItems.InitItemUdevRule(LanItems.current) if new_rule
+
       case based_on
       when :mac
+        return if new_rule
+
         LanItems.RemoveItemUdev("ATTR{dev_port}")
 
         # FIXME: While the user is able to modify the udev rule using the
@@ -482,7 +492,7 @@ def update_item_udev_rule!(based_on = :mac)
         LanItems.ReplaceItemUdev(
           "KERNELS",
           "ATTR{address}",
-          LanItems.getCurrentItem.fetch("hwinfo", {}).fetch("mac", "")
+          LanItems.getCurrentItem.fetch("hwinfo", {}).fetch("permanent_mac", "")
         )
       when :bus_id
         # Update or insert the dev_port if the sysfs dev_port attribute is present
@@ -521,7 +531,6 @@ def ReplaceItemUdev(replace_key, new_key, new_val)
       # =    for assignment
       # ==   for equality checks
       operator = new_key == "NAME" ? "=" : "=="
-      current_rule = GetItemUdevRule(@current)
       rule = RemoveKeyFromUdevRule(getUdevFallback, replace_key)
 
       # NAME="devname" has to be last in the rule.
@@ -531,7 +540,7 @@ def ReplaceItemUdev(replace_key, new_key, new_val)
       new_rule = AddToUdevRule(rule, "#{new_key}#{operator}\"#{new_val}\"")
       new_rule.push(name_tuple)
 
-      if current_rule.sort != new_rule.sort
+      if current_udev_rule.sort != new_rule.sort
         SetModified()
 
         log.info("ReplaceItemUdev: new udev rule = #{new_rule}")
@@ -1078,7 +1087,7 @@ def getDeviceName(oldname)
 
       hardware.each do |hw|
         hw_dev_name = hw["dev_name"] || ""
-        hw_dev_mac = hw["mac"] || ""
+        hw_dev_mac = hw["permanent_mac"] || ""
         hw_dev_busid = hw["busid"] || ""
 
         case oldname
@@ -1548,11 +1557,11 @@ def BuildLanOverview
         conn = HTML.Bold(format("(%s)", _("Not connected"))) if !item_hwinfo["link"]
         conn = HTML.Bold(format("(%s)", _("No hwinfo"))) if item_hwinfo.empty?
 
-        mac_dev = HTML.Bold("MAC : ") + item_hwinfo["mac"].to_s + "<br>"
+        mac_dev = HTML.Bold("MAC : ") + item_hwinfo["permanent_mac"].to_s + "<br>"
         bus_id  = HTML.Bold("BusID : ") + item_hwinfo["busid"].to_s + "<br>"
         physical_port_id = HTML.Bold("PhysicalPortID : ") + physical_port_id(ifcfg_name) + "<br>"
 
-        rich << " " << conn << "<br>" << mac_dev if IsNotEmpty(item_hwinfo["mac"])
+        rich << " " << conn << "<br>" << mac_dev if IsNotEmpty(item_hwinfo["permanent_mac"])
         rich << bus_id if IsNotEmpty(item_hwinfo["busid"])
         rich << physical_port_id if physical_port_id?(ifcfg_name)
         # display it only if we need it, don't duplicate "ifcfg_name" above

test/lan_items_helpers_test.rb

@@ -156,8 +156,8 @@ def udev_rule(mac, name)
         },
         1 => {
           "hwinfo" => {
-            "mac"      => "00:00:00:00:00:01",
-            "dev_name" => "eth1"
+            "permanent_mac" => "00:00:00:00:00:01",
+            "dev_name"      => "eth1"
           },
           # always exists
           "udev"   => {
@@ -205,7 +205,7 @@ def check_GetItemUdev(key, expected_value)
     before(:each) do
       allow(Yast::LanItems)
         .to receive(:GetLanItem)
-        .and_return("hwinfo" => { "mac" => MAC }, "ifcfg" => "eth0")
+        .and_return("hwinfo" => { "permanent_mac" => MAC }, "ifcfg" => "eth0")
     end
 
     it "returns proper value when key exists" do
@@ -249,8 +249,24 @@ def check_GetItemUdev(key, expected_value)
   end
 end
 
+describe "#current_udev_rule" do
+  let(:busid) { "0000:08:00.0" }
+  let(:hwinfo) { { "dev_name" => "test0", "busid" => busid, "permanent_mac" => "24:be:05:ce:1e:91" } }
+  let(:udev_net) { ["KERNELS==\"#{busid}\"", "NAME=\"test0\""] }
+  let(:rule) { { 0 => { "hwinfo" => hwinfo, "udev" => { "net" => udev_net } } } }
+
+  before do
+    Yast::LanItems.Items = rule
+    Yast::LanItems.current = 0
+  end
+
+  it "returns the current item udev rule" do
+    expect(Yast::LanItems.current_udev_rule).to contain_exactly("KERNELS==\"#{busid}\"", "NAME=\"test0\"")
+  end
+end
+
 describe "#update_item_udev_rule!" do
-  let(:hwinfo) { { "dev_name" => "test0", "busid" => "0000:08:00.0", "mac" => "24:be:05:ce:1e:91" } }
+  let(:hwinfo) { { "dev_name" => "test0", "busid" => "0000:08:00.0", "permanent_mac" => "24:be:05:ce:1e:91" } }
   let(:udev_net) { ["ATTR{address}==\"24:be:05:ce:1e:91\"", "KERNEL==\"eth*\"", "NAME=\"test0\""] }
   let(:rule) { { 0 => { "hwinfo" => hwinfo, "udev" => { "net" => udev_net } } } }
 

test/lan_udev_auto_test.rb

@@ -26,9 +26,10 @@
     allow(lan_items).to receive(:ReadHardware) {
       [
         {
-          "dev_name" => NEW_STYLE_NAME,
-          "mac"      => "00:11:22:33:44:FF",
-          "busid"    => "0000:00:19.0"
+          "dev_name"      => NEW_STYLE_NAME,
+          "mac"           => "00:11:22:33:44:FF",
+          "permanent_mac" => "00:11:22:33:44:FF",
+          "busid"         => "0000:00:19.0"
         }
       ]
     }

test/lib/network/edit_nic_name_test.rb

@@ -12,7 +12,7 @@
   let(:current_name) { "spec0" }
   let(:new_name) { "new1" }
   let(:existing_new_name) { "existing_new_name" }
-  let(:interface_hwinfo) { { "dev_name" => current_name, "mac" => "00:01:02:03:04:05" } }
+  let(:interface_hwinfo) { { "dev_name" => current_name, "permanent_mac" => "00:01:02:03:04:05" } }
 
   describe "#run" do
     # general mocking stuff is placed here
@@ -92,6 +92,12 @@
           expect(Yast::Routing.devices).to include(new_name)
         end
 
+        context "but used the same matching udev key" do
+          it "does not touch the current udev rule" do
+            expect(Yast::LanItems).to_not receive(:update_item_udev_rule!)
+          end
+        end
+
         context "and there are some routes referencing the previous name" do
           before do
             allow(Yast::Routing).to receive(:device_routes?).with(current_name).and_return(true)
@@ -118,6 +124,18 @@
             subject.run
           end
         end
+
+        context "having modified the matching udev key" do
+          before(:each) do
+            # emulate UI work
+            allow(UI).to receive(:QueryWidget).with(:dev_name, :Value) { current_name }
+            allow(UI).to receive(:QueryWidget).with(:udev_type, :CurrentButton) { :bus_id }
+          end
+
+          it "updates the current udev rule with the key used" do
+            expect(Yast::LanItems).to_not receive(:update_item_udev_rule!).with(:bus_id)
+          end
+        end
       end
 
       context "and closed canceling the changes" do
@@ -127,7 +145,6 @@
           expect(subject.run).to be_equal current_name
         end
       end
-
     end
   end
 end

test/netcard_test.rb

@@ -40,6 +40,7 @@
       "bus"           => HWINFO_DEVICE_BUS,
       "busid"         => HWINFO_DEVICE_BUSID,
       "mac"           => HWINFO_DEVICE_MAC,
+      "permanent_mac" => HWINFO_DEVICE_MAC,
       "link"          => nil,
       "wl_channels"   => nil,
       "wl_bitrates"   => nil,

test/network_autoyast_test.rb

@@ -453,19 +453,22 @@ def mock_lan_item(renamed_to: nil)
       let(:hw_netcard) do
         [
           {
-            "dev_name" => "eth0",
-            "busid"    => "0000:01:00.0",
-            "mac"      => "00:00:00:00:00:00"
+            "dev_name"      => "eth0",
+            "busid"         => "0000:01:00.0",
+            "mac"           => "00:00:00:00:00:00",
+            "permanent_mac" => "00:00:00:00:00:00"
           },
           {
-            "dev_name" => "eth1",
-            "busid"    => "0000:01:00.1",
-            "mac"      => "00:00:00:00:00:01"
+            "dev_name"      => "eth1",
+            "busid"         => "0000:01:00.1",
+            "mac"           => "00:00:00:00:00:01",
+            "permanent_mac" => "00:00:00:00:00:01"
           },
           {
-            "dev_name" => "eth2",
-            "busid"    => "0000:01:00.2",
-            "mac"      => "00:00:00:00:00:02"
+            "dev_name"      => "eth2",
+            "busid"         => "0000:01:00.2",
+            "mac"           => "00:00:00:00:00:02",
+            "permanent_mac" => "00:00:00:00:00:02"
           }
         ]
       end