Merge pull request #358 from yast/fix_activating_mdrad

Fix activating mdrad

package/yast2-bootloader.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Tue Aug 30 08:42:25 UTC 2016 - jreidinger@suse.com
+
+- fix crash when activating partition on md raid (bnc#995627)
+- 3.1.202
+
 -------------------------------------------------------------------
 Fri Aug 26 09:25:49 UTC 2016 - jsrain@suse.cz
 

package/yast2-bootloader.spec

@@ -17,7 +17,7 @@
 
 
 Name:           yast2-bootloader
-Version:        3.1.201
+Version:        3.1.202
 Release:        0
 
 BuildRoot:      %{_tmppath}/%{name}-%{version}-build

src/lib/bootloader/mbr_update.rb

@@ -1,6 +1,7 @@
 require "yast"
 
 require "bootloader/boot_record_backup"
+require "bootloader/stage1_device"
 require "yast2/execute"
 
 Yast.import "Arch"
@@ -45,7 +46,7 @@ def create_backups
     end
 
     def mbr_is_gpt?
-      mbr_storage_object = Yast::Storage.GetTargetMap[mbr_disk]
+      mbr_storage_object = target_map[mbr_disk]
       raise "Cannot find in storage mbr disk #{mbr_disk}" unless mbr_storage_object
       mbr_type = mbr_storage_object["label"]
       log.info "mbr type = #{mbr_type}"
@@ -138,24 +139,35 @@ def disks_to_rewrite
       ret = [mbr_disk]
       # Add to disks only if part of raid on base devices lives on mbr_disk
       ret.concat(mbrs) if mbrs.include?(mbr_disk)
+      # get only real disks
+      ret = ret.each_with_object([]) do |disk, res|
+        res.concat(::Bootloader::Stage1Device.new(disk).real_devices)
+      end
 
       ret.uniq
     end
 
     def first_base_device_to_boot(md_device)
-      md = Yast::BootStorage.Md2Partitions(md_device)
-      md.reduce do |res, items|
-        device, bios_id = items
-        next device unless res
+      md = ::Bootloader::Stage1Device.new(md_device)
+      md.real_devices.min_by { |device| bios_id_for(device) }
+    end
 
-        bios_id < md[res] ? device : res
-      end
+    MAX_BIOS_ID = 1000
+    def bios_id_for(device)
+      disk = Yast::Storage.GetDiskPartition(device)["disk"]
+      disk_info = target_map[disk]
+      return MAX_BIOS_ID unless disk_info
+
+      bios_id = disk_info["bios_id"]
+      # prefer device without bios id over ones without disk info
+      return MAX_BIOS_ID - 1  if !bios_id || bios_id !~ /0x[0-9a-fA-F]+/
+
+      bios_id[2..-1].to_i(16) - 0x80
     end
 
     # List of partition for disk that can be used for setting boot flag
     def activatable_partitions(disk)
-      tm = Yast::Storage.GetTargetMap
-      partitions = tm.fetch(disk, {}).fetch("partitions", [])
+      partitions = target_map.fetch(disk, {}).fetch("partitions", [])
       # do not select swap and do not select BIOS grub partition
       # as it clear its special flags (bnc#894040)
       partitions.select do |p|
@@ -173,28 +185,20 @@ def extended_partition_num(disk)
     end
 
     # Given a device name to which we install the bootloader (loader_device),
-    # get the name of the partition which should be activated.
-    # Also return the device file name of the disk device that corresponds to
-    # loader_device (i.e. where the corresponding MBR can be found).
+    # gets back disk and partition number to activate. If empty Hash is returned
+    # then no suitable partition to activate found.
     # @param [String] loader_device string the device to install bootloader to
-    # @return a map $[ "mbr": string, "num": any]
-    #  containing device (eg. "/dev/hda4"), disk (eg. "/dev/hda") and
-    #  partition number (eg. 4)
+    # @return a Hash `{ "mbr" => String, "num" => Integer }`
+    #  containing disk (eg. "/dev/hda") and partition number (eg. 4)
     def partition_to_activate(loader_device)
-      p_dev = Yast::Storage.GetDiskPartition(loader_device)
+      real_device = first_base_device_to_boot(loader_device)
+      log.info "real devices for #{loader_device} is #{real_device}"
+
+      p_dev = Yast::Storage.GetDiskPartition(real_device)
       num = p_dev["nr"].to_i
       mbr_dev = p_dev["disk"]
       raise "Invalid loader device #{loader_device}" unless mbr_dev
 
-      # If loader_device is /dev/md* (which means bootloader is installed to
-      # /dev/md*), then call recursive method with partition that lays on device
-      # with the lowest bios id number or first one if noone have bios id
-      # FIXME: use ::storage to detect md devices, not by name!
-      if loader_device.start_with?("/dev/md")
-        base_device = first_base_device_to_boot(loader_device)
-        return partition_to_activate(base_device) if base_device
-      end
-
       # (bnc # 337742) - Unable to boot the openSUSE (32 and 64 bits) after installation
       # if loader_device is disk Choose any partition which is not swap to
       # satisfy such bios (bnc#893449)
@@ -234,5 +238,9 @@ def partitions_to_activate
 
       result.uniq
     end
+
+    def target_map
+      @target_map ||= Yast::Storage.GetTargetMap
+    end
   end
 end

src/modules/BootStorage.rb

@@ -325,7 +325,6 @@ def encrypted_boot?
     publish :variable => :BootPartitionDevice, :type => "string"
     publish :variable => :RootPartitionDevice, :type => "string"
     publish :variable => :ExtendedPartitionDevice, :type => "string"
-    publish :function => :Md2Partitions, :type => "map <string, integer> (string)"
   end
 
   BootStorage = BootStorageClass.new

test/mbr_update_test.rb

@@ -119,6 +119,20 @@ def stage1(devices: [], activate: false, generic_mbr: false)
         subject.run(stage1(generic_mbr: true))
       end
 
+      it "always uses real devices" do
+        allow(Yast::BootStorage).to receive(:mbr_disk)
+          .and_return("/dev/md0")
+
+        allow(::Bootloader::Stage1Device).to receive(:new).with("/dev/md0")
+          .and_return(double(real_devices: ["/dev/sda1", "/dev/sdb1"]))
+        expect(Yast::Execute).to receive(:on_target).at_least(:twice) do |*args|
+          next nil unless args.first =~ /dd/
+          next nil unless args.include?("of=/dev/sdb")
+          expect(args).to be_include("of=/dev/sda")
+        end
+        subject.run(stage1(generic_mbr: true))
+      end
+
       it "install syslinux if non on initial stage" do
         allow(Yast::Stage).to receive(:initial).and_return(false)
         expect(Yast::PackageSystem).to receive(:Install).with("syslinux")
@@ -188,6 +202,20 @@ def stage1(devices: [], activate: false, generic_mbr: false)
 
           subject.run(stage1(activate: true, devices: ["/dev/sda1", "/dev/sdb1"]))
         end
+
+        it "sets boot flag on boot device with the lowest bios id when stage1 partition is on md" do
+          allow(Yast::Storage).to receive(:GetTargetMap).and_return(
+            "/dev/sda" => { "label" => "msdos", "bios_id" => "0x81" },
+            "/dev/sdb" => { "label" => "msdos", "bios_id" => "0x80" }
+          )
+
+          allow(::Bootloader::Stage1Device).to receive(:new).with("/dev/md1")
+            .and_return(double(real_devices: ["/dev/sda1", "/dev/sdb1"]))
+          expect(Yast::Execute).to receive(:locally)
+            .with(/parted/, "-s", "/dev/sdb", "set", 1, "boot", "on")
+
+          subject.run(stage1(activate: true, devices: ["/dev/md1"]))
+        end
       end
 
       context "disk label is GPT" do