From 2d3297b3521a723856e7c23631b674a767937364 Mon Sep 17 00:00:00 2001 From: Josef Reidinger Date: Tue, 30 Aug 2016 10:42:11 +0200 Subject: [PATCH 1/5] fix unknown function when activating partition on md raid (bnc#995627) --- src/lib/bootloader/mbr_update.rb | 53 ++++++++++++++++++++------------ src/modules/BootStorage.rb | 1 - test/mbr_update_test.rb | 13 ++++++++ 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/lib/bootloader/mbr_update.rb b/src/lib/bootloader/mbr_update.rb index feb302432..59a50b96d 100644 --- a/src/lib/bootloader/mbr_update.rb +++ b/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}" @@ -142,20 +143,36 @@ def disks_to_rewrite ret.uniq end + MAX_BIOS_ID = 1000 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 - - bios_id < md[res] ? device : res + md = ::Bootloader::Stage1Device.new(md_device) + lowest_id = MAX_BIOS_ID + md.real_devices.reduce do |res, device| + res ||= device # ensure at least one device is used, even if none have bios_id + bios_id_num = bios_id_for(device) + if lowest_id > bios_id_num + lowest_id = bios_id_num + next device + else + res + end end end + 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"] + return MAX_BIOS_ID 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| @@ -181,20 +198,14 @@ def extended_partition_num(disk) # containing device (eg. "/dev/hda4"), 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 +245,9 @@ def partitions_to_activate result.uniq end + + def target_map + @target_map ||= Yast::Storage.GetTargetMap + end end end diff --git a/src/modules/BootStorage.rb b/src/modules/BootStorage.rb index efd972349..aaca48192 100644 --- a/src/modules/BootStorage.rb +++ b/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)" end BootStorage = BootStorageClass.new diff --git a/test/mbr_update_test.rb b/test/mbr_update_test.rb index 8b17f78dc..41ae4eb7c 100644 --- a/test/mbr_update_test.rb +++ b/test/mbr_update_test.rb @@ -188,6 +188,19 @@ 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") { |d| 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 From ef30a230fca28b07932c23234c93f2abc865e128 Mon Sep 17 00:00:00 2001 From: Josef Reidinger Date: Tue, 30 Aug 2016 10:42:48 +0200 Subject: [PATCH 2/5] Changes --- package/yast2-bootloader.changes | 6 ++++++ package/yast2-bootloader.spec | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/package/yast2-bootloader.changes b/package/yast2-bootloader.changes index e304702df..1cc9fc4b1 100644 --- a/package/yast2-bootloader.changes +++ b/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 diff --git a/package/yast2-bootloader.spec b/package/yast2-bootloader.spec index 330e7812d..61ff823c5 100644 --- a/package/yast2-bootloader.spec +++ b/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 From fac2a8f4f0038c8b93ac5128a35bf8e8e58279ac Mon Sep 17 00:00:00 2001 From: Josef Reidinger Date: Tue, 30 Aug 2016 10:47:46 +0200 Subject: [PATCH 3/5] make rubocop happy --- test/mbr_update_test.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/mbr_update_test.rb b/test/mbr_update_test.rb index 41ae4eb7c..74420a725 100644 --- a/test/mbr_update_test.rb +++ b/test/mbr_update_test.rb @@ -195,7 +195,8 @@ def stage1(devices: [], activate: false, generic_mbr: false) "/dev/sdb" => { "label" => "msdos", "bios_id" => "0x80" } ) - allow(::Bootloader::Stage1Device).to receive(:new).with("/dev/md1") { |d| double(real_devices: ["/dev/sda1","/dev/sdb1"]) } + 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") From 07837c30f63abc3f92efae5609c6874f698afe9b Mon Sep 17 00:00:00 2001 From: Josef Reidinger Date: Tue, 30 Aug 2016 12:28:02 +0200 Subject: [PATCH 4/5] changes from review --- src/lib/bootloader/mbr_update.rb | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/lib/bootloader/mbr_update.rb b/src/lib/bootloader/mbr_update.rb index 59a50b96d..14991c638 100644 --- a/src/lib/bootloader/mbr_update.rb +++ b/src/lib/bootloader/mbr_update.rb @@ -143,29 +143,20 @@ def disks_to_rewrite ret.uniq end - MAX_BIOS_ID = 1000 def first_base_device_to_boot(md_device) md = ::Bootloader::Stage1Device.new(md_device) - lowest_id = MAX_BIOS_ID - md.real_devices.reduce do |res, device| - res ||= device # ensure at least one device is used, even if none have bios_id - bios_id_num = bios_id_for(device) - if lowest_id > bios_id_num - lowest_id = bios_id_num - next device - else - res - end - end + md.real_devices.min_by { |device| bios_id_for(device) } 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"] - return MAX_BIOS_ID if !bios_id || bios_id !~ /0x[0-9a-fA-F]+/ + # 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 @@ -190,13 +181,11 @@ 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) real_device = first_base_device_to_boot(loader_device) log.info "real devices for #{loader_device} is #{real_device}" From e99a3a79e978f2c48ea75a22dba10b041b59af88 Mon Sep 17 00:00:00 2001 From: Josef Reidinger Date: Tue, 30 Aug 2016 13:45:38 +0200 Subject: [PATCH 5/5] always use real devices for generic mbr (found during manual testing) --- src/lib/bootloader/mbr_update.rb | 4 ++++ test/mbr_update_test.rb | 14 ++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/lib/bootloader/mbr_update.rb b/src/lib/bootloader/mbr_update.rb index 14991c638..8819e53d6 100644 --- a/src/lib/bootloader/mbr_update.rb +++ b/src/lib/bootloader/mbr_update.rb @@ -139,6 +139,10 @@ 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 diff --git a/test/mbr_update_test.rb b/test/mbr_update_test.rb index 74420a725..6b588069a 100644 --- a/test/mbr_update_test.rb +++ b/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")