From 457047693c6b7da5565ce7fab30fae53882610eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 28 Aug 2018 18:16:03 +0100 Subject: [PATCH 1/6] Ensure system fulfilling the requirements Basically, avoiding to continue writing the configuration and reverting the sysconfig when user rejects the installation of required package. --- src/lib/bootloader/bootloader_base.rb | 29 ++++++++++-- src/modules/Bootloader.rb | 19 +++++++- test/bootloader_base_test.rb | 64 +++++++++++++++++++++++---- test/bootloader_test.rb | 25 +++++++++++ test/grub2_efi_test.rb | 17 +++++-- test/grub2_test.rb | 3 +- test/grub2base_test.rb | 4 +- 7 files changed, 141 insertions(+), 20 deletions(-) diff --git a/src/lib/bootloader/bootloader_base.rb b/src/lib/bootloader/bootloader_base.rb index a9f80debf..e619b1472 100644 --- a/src/lib/bootloader/bootloader_base.rb +++ b/src/lib/bootloader/bootloader_base.rb @@ -12,13 +12,29 @@ class BootloaderBase def initialize @read = false @proposed = false + @initial_sysconfig = Sysconfig.from_system + end + + # Prepares the system to (before write the configuration) + # + # Writes the new sysconfig and, when the Mode.normal is set, tries to install the required packages. + # If user decides to cancel the installation, it restores the previous sysconfig. + # + # @return [Boolean] true whether the system could be prepared as expected; + # false when user cancel the installation of needed packages + def prepare + write_sysconfig + + return true unless Yast::Mode.normal + return true if Yast::PackageSystem.InstallAll(packages) + + restore_initial_sysconfig + + false end # writes configuration to target disk def write - write_sysconfig - # in running system install package, for other modes, it need specific handling - Yast::PackageSystem.InstallAll(packages) if Yast::Mode.normal end # reads configuration from target disk @@ -66,6 +82,13 @@ def write_sysconfig(prewrite: false) prewrite ? sysconfig.pre_write : sysconfig.write end + # Writes the sysconfig readed in the initialization + # + # Useful to "rollback" sysconfig changes if something is fails before finish the configuration + def restore_initial_sysconfig + @initial_sysconfig.write + end + # merges other bootloader configuration into this one. # It have to be same bootloader type. def merge(other) diff --git a/src/modules/Bootloader.rb b/src/modules/Bootloader.rb index dbb6cd585..17dac59aa 100644 --- a/src/modules/Bootloader.rb +++ b/src/modules/Bootloader.rb @@ -16,6 +16,7 @@ # $Id$ # require "yast" +require "yast2/popup" require "bootloader/exceptions" require "bootloader/sysconfig" require "bootloader/bootloader_factory" @@ -228,12 +229,16 @@ def Write # run Progress bar stages = [ + # progress stage, text in dialog (short) + _("Prepare system"), # progress stage, text in dialog (short) _("Create initrd"), # progress stage, text in dialog (short) _("Save boot loader configuration") ] titles = [ + # progress step, text in dialog (short) + _("Preparing system..."), # progress step, text in dialog (short) _("Creating initrd..."), # progress step, text in dialog (short) @@ -255,12 +260,22 @@ def Write Progress.Title(titles[0]) end + progress_state = Progress.set(false) + if !::Bootloader::BootloaderFactory.current.prepare + log.error "System could not be prepared successfully, required packages were not installed" + Yast2::Popup.show(_("Cannot continue without install required packages")) + return false + end + Progress.set(progress_state) + Progress.NextStage + Progress.Title(titles[1]) unless Mode.normal + ret = write_initrd log.error "Error occurred while creating initrd" unless ret - Progress.NextStep - Progress.Title(titles[1]) unless Mode.normal + Progress.NextStage + Progress.Title(titles[2]) unless Mode.normal ::Bootloader::BootloaderFactory.current.write diff --git a/test/bootloader_base_test.rb b/test/bootloader_base_test.rb index 9517ea1fb..c882d3d53 100644 --- a/test/bootloader_base_test.rb +++ b/test/bootloader_base_test.rb @@ -3,28 +3,76 @@ require "bootloader/bootloader_base" describe Bootloader::BootloaderBase do - describe "#write" do + describe "#prepare" do + let(:initial_sysconfig) { double(Bootloader::Sysconfig, write: nil) } + let(:new_sysconfig) { double(Bootloader::Sysconfig, write: nil) } + let(:bootloader) { "funny_bootloader" } + let(:normal_mode) { false } + before do - allow(Bootloader::Sysconfig).to receive(:new).and_return(double(write: nil)) + allow(Yast::Mode).to receive(:normal).and_return(normal_mode) + + allow(Bootloader::Sysconfig).to receive(:new).and_return(new_sysconfig) + allow(Bootloader::Sysconfig).to receive(:from_system).and_return(initial_sysconfig) + allow(Yast::PackageSystem).to receive(:InstallAll) + allow(Yast2::Popup).to receive(:show).and_return(true) + subject.define_singleton_method(:name) { "funny_bootloader" } end it "writes to sysconfig name of its child" do - sysconfig = double(Bootloader::Sysconfig, write: nil) expect(Bootloader::Sysconfig).to receive(:new) .with(bootloader: "funny_bootloader") - .and_return(sysconfig) + .and_return(new_sysconfig) - subject.write + subject.prepare end - context "Mode.normal is set" do - it "install packages required by bootloader" do + context "when is not Mode.normal" do + it "returns true" do + expect(subject.prepare).to eq(true) + end + end + + context "when is Mode.normal" do + let(:normal_mode) { true } + + it "tries to install required packages" do expect(Yast::PackageSystem).to receive(:InstallAll).with(["kexec-tools"]) - subject.write + subject.prepare + end + + context "and the user accepts the installation" do + before do + allow(Yast::PackageSystem).to receive(:InstallAll).with(["kexec-tools"]).and_return(true) + end + + it "returns true" do + expect(subject.prepare).to eq(true) + end + + it "does not rollback the sysconfig" do + expect(initial_sysconfig).to_not receive(:write) + end + end + + context "and the user does not accept the installation" do + before do + allow(Yast::PackageSystem).to receive(:InstallAll).with(["kexec-tools"]).and_return(false) + end + + it "restores the initial sysconfig" do + expect(initial_sysconfig).to receive(:write) + + subject.prepare + end + + it "returns false" do + expect(subject.prepare).to eq(false) + end end end end diff --git a/test/bootloader_test.rb b/test/bootloader_test.rb index dcba32ee2..05e6c12f7 100644 --- a/test/bootloader_test.rb +++ b/test/bootloader_test.rb @@ -22,6 +22,31 @@ def kernel_line(target) allow(::Bootloader::BootloaderFactory.current).to receive(:read) end + describe ".Write" do + context "when user cancels the installation of required packages" do + before do + allow(Yast::PackageSystem).to receive(:InstallAll).and_return(false) + allow(Yast2::Popup).to receive(:show) + end + + it "shows an information message" do + expect(Yast2::Popup).to receive(:show) + + subject.Write + end + + it "returns false" do + expect(subject.Write).to eq(false) + end + + it "logs an error" do + expect(subject.log).to receive(:error).with(/could not be prepared/) + + subject.Write + end + end + end + describe ".Import" do before do end diff --git a/test/grub2_efi_test.rb b/test/grub2_efi_test.rb index 0cc2e3cac..b6c6c889e 100644 --- a/test/grub2_efi_test.rb +++ b/test/grub2_efi_test.rb @@ -22,7 +22,7 @@ end end - describe "write" do + describe "#write" do it "setups protective mbr to real disks containing /boot/efi" do subject.pmbr_action = :add allow(Yast::BootStorage).to receive(:gpt_boot_disk?).and_return(true) @@ -45,19 +45,28 @@ subject.write end + end + + describe "#prepare" do + let(:sysconfig) { double(Bootloader::Sysconfig) } + + before do + allow(Bootloader::Sysconfig).to receive(:from_system) + allow(Yast::PackageSystem).to receive(:InstallAll).and_return(true) + end + it "writes secure boot and trusted boot configuration to bootloader sysconfig" do # This test fails (only!) in Travis with # Failure/Error: subject.write Storage::Exception: Storage::Exception - sysconfig = double(Bootloader::Sysconfig) - expect(sysconfig).to receive(:write) expect(Bootloader::Sysconfig).to receive(:new) .with(bootloader: "grub2-efi", secure_boot: true, trusted_boot: true) .and_return(sysconfig) + expect(sysconfig).to receive(:write) subject.secure_boot = true subject.trusted_boot = true - subject.write + subject.prepare end end diff --git a/test/grub2_test.rb b/test/grub2_test.rb index 7070898e4..5f6c82071 100644 --- a/test/grub2_test.rb +++ b/test/grub2_test.rb @@ -37,8 +37,9 @@ end describe "write" do + let(:stage1) { double(Bootloader::Stage1, devices: [], generic_mbr?: false, write: nil) } + before do - stage1 = double(Bootloader::Stage1, devices: [], generic_mbr?: false, write: nil) allow(Bootloader::Stage1).to receive(:new).and_return(stage1) allow(Bootloader::MBRUpdate).to receive(:new).and_return(double(run: nil)) allow(Bootloader::GrubInstall).to receive(:new).and_return(double.as_null_object) diff --git a/test/grub2base_test.rb b/test/grub2base_test.rb index 0e9d785c2..69b68b53b 100644 --- a/test/grub2base_test.rb +++ b/test/grub2base_test.rb @@ -30,14 +30,14 @@ it "reads trusted boot configuration from sysconfig" do mocked_sysconfig = ::Bootloader::Sysconfig.new(trusted_boot: true) - expect(::Bootloader::Sysconfig).to receive(:from_system).and_return(mocked_sysconfig) + allow(::Bootloader::Sysconfig).to receive(:from_system).and_return(mocked_sysconfig) subject.read expect(subject.trusted_boot).to eq true mocked_sysconfig = ::Bootloader::Sysconfig.new(trusted_boot: false) - expect(::Bootloader::Sysconfig).to receive(:from_system).and_return(mocked_sysconfig) + allow(::Bootloader::Sysconfig).to receive(:from_system).and_return(mocked_sysconfig) subject.read From 95f2ded2ec940829772f216c71dc0c6c2d7b63a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 29 Aug 2018 08:44:19 +0100 Subject: [PATCH 2/6] Add test for the inclusion of trustedgrub2 packages --- test/grub2_test.rb | 75 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 9 deletions(-) diff --git a/test/grub2_test.rb b/test/grub2_test.rb index 5f6c82071..d2fc5b9d2 100644 --- a/test/grub2_test.rb +++ b/test/grub2_test.rb @@ -136,24 +136,81 @@ end describe "#packages" do - it "return list containing grub2 package" do + let(:initial_stage) { true } + let(:generic_mbr) { true } + let(:stage1) { double(generic_mbr?: generic_mbr) } + + before do + allow(Yast::Stage).to receive(:initial).and_return(initial_stage) + allow(Bootloader::Stage1).to receive(:new).and_return(stage1) + end + + it "contains grub2 package" do expect(subject.packages).to include("grub2") end - it "returns list containing syslinux package if generic_mbr is used" do - stage1 = double(generic_mbr?: true) - allow(Bootloader::Stage1).to receive(:new).and_return(stage1) + context "when is in initial stage" do + it "does not include syslinux" do + expect(subject.packages).to_not include("syslinux") + end + end + + context "when is not in initial stage" do + let(:initial_stage) { false } + + context "and generic_mbr is used" do + it "contains syslinux package" do + expect(subject.packages).to include("syslinux") + end + end + + context "and generic_mbr is not used" do + let(:generic_mbr) { false } - expect(subject.packages).to include("syslinux") + it "contains syslinux package" do + expect(subject.packages).to_not include("syslinux") + end + end end - it "returns list without syslinux package if generic_mbr is not used" do - stage1 = double(generic_mbr?: false) - allow(Bootloader::Stage1).to receive(:new).and_return(stage1) + context "when trusted boot is required" do + before do + allow(subject).to receive(:trusted_boot).and_return(true) + end + + context "and is x86_64 architecture" do + before do + allow(Yast::Arch).to receive(:x86_64).and_return(true) + end + + it "contains trustedgrub2 packages" do + expect(subject.packages).to include("trustedgrub2") + expect(subject.packages).to include("trustedgrub2-i386-pc") + end + end + + context "and is i386 architecture" do + before do + allow(Yast::Arch).to receive(:x86_64).and_return(true) + end - expect(subject.packages).to_not include("syslinux") + it "contains trustedgrub2 packages" do + expect(subject.packages).to include("trustedgrub2") + expect(subject.packages).to include("trustedgrub2-i386-pc") + end + end end + context "when trusted boot is not required" do + before do + allow(subject).to receive(:trusted_boot).and_return(false) + end + + it "does not contain the trusged grub packages" do + expect(subject.packages).to_not include("trustedgrub2") + expect(subject.packages).to_not include("trustedgrub2-i386-pc") + end + end end describe "#summary" do From f5d6d6946fe64b1fa5aa28d61b9eb5edacc8bf46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 28 Aug 2018 18:32:56 +0100 Subject: [PATCH 3/6] Minor improvements --- src/lib/bootloader/bootloader_base.rb | 34 ++++++++++++++++----------- src/lib/bootloader/grub2.rb | 33 ++++++++++++++++---------- src/lib/bootloader/write_dialog.rb | 8 ++++--- src/modules/Bootloader.rb | 30 +++++++---------------- 4 files changed, 54 insertions(+), 51 deletions(-) diff --git a/src/lib/bootloader/bootloader_base.rb b/src/lib/bootloader/bootloader_base.rb index e619b1472..2ee68c056 100644 --- a/src/lib/bootloader/bootloader_base.rb +++ b/src/lib/bootloader/bootloader_base.rb @@ -17,8 +17,8 @@ def initialize # Prepares the system to (before write the configuration) # - # Writes the new sysconfig and, when the Mode.normal is set, tries to install the required packages. - # If user decides to cancel the installation, it restores the previous sysconfig. + # Writes the new sysconfig and, when the Mode.normal is set, tries to install the required + # packages. If user decides to cancel the installation, it restores the previous sysconfig. # # @return [Boolean] true whether the system could be prepared as expected; # false when user cancel the installation of needed packages @@ -66,11 +66,8 @@ def proposed? def packages res = [] - # added kexec-tools fate# 303395 - if !Yast::Mode.live_installation && - Yast::Linuxrc.InstallInf("kexec_reboot") != "0" - res << "kexec-tools" - end + # added kexec-tools fate#303395 + res << "kexec-tools" if include_kexec_tools_package? res end @@ -82,13 +79,6 @@ def write_sysconfig(prewrite: false) prewrite ? sysconfig.pre_write : sysconfig.write end - # Writes the sysconfig readed in the initialization - # - # Useful to "rollback" sysconfig changes if something is fails before finish the configuration - def restore_initial_sysconfig - @initial_sysconfig.write - end - # merges other bootloader configuration into this one. # It have to be same bootloader type. def merge(other) @@ -97,5 +87,21 @@ def merge(other) @read ||= other.read? @proposed ||= other.proposed? end + + private + + # @return [Boolean] true when kexec-tools package should be included; false otherwise + def include_kexec_tools_package? + return false if Yast::Mode.live_installation + + Yast::Linuxrc.InstallInf("kexec_reboot") != "0" + end + + # Writes the sysconfig readed in the initialization + # + # Useful to "rollback" sysconfig changes if something is fails before finish the configuration + def restore_initial_sysconfig + @initial_sysconfig.write + end end end diff --git a/src/lib/bootloader/grub2.rb b/src/lib/bootloader/grub2.rb index dcce9182f..a75f25bc3 100644 --- a/src/lib/bootloader/grub2.rb +++ b/src/lib/bootloader/grub2.rb @@ -132,19 +132,9 @@ def name def packages res = super - res << "grub2" - - # do not require it in insts-sys as insts-sys have it itself (bsc#1004229) - if stage1.generic_mbr? && !Yast::Stage.initial - # needed for generic _mbr binary files - res << "syslinux" - end - - if Yast::Arch.x86_64 || Yast::Arch.i386 - res << "trustedgrub2" << "trustedgrub2-i386-pc" if trusted_boot - end - + res << "syslinux" if include_syslinux_package? + res << "trustedgrub2" << "trustedgrub2-i386-pc" if include_trustedgrub2_packages? res end @@ -157,6 +147,25 @@ def write_sysconfig(prewrite: false) private + # Checks if syslinux package should be included + # + # Needed for generic_mbr binary files, but it must not be required in inst-sys as inst-sys have + # it itself (bsc#1004229). + # + # @return [Boolean] true if syslinux package should be included; false otherwise + def include_syslinux_package? + return false if Yast::Stage.initial + + stage1.generic_mbr? + end + + # @return [Boolean] true when trustedgrub2 packages should be included; false otherwise + def include_trustedgrub2_packages? + return false unless trusted_boot + + Yast::Arch.x86_64 || Yast::Arch.i386 + end + def devicegraph Y2Storage::StorageManager.instance.staging end diff --git a/src/lib/bootloader/write_dialog.rb b/src/lib/bootloader/write_dialog.rb index dd8a356ab..c301bfd12 100644 --- a/src/lib/bootloader/write_dialog.rb +++ b/src/lib/bootloader/write_dialog.rb @@ -9,11 +9,13 @@ class WriteDialog include Yast::I18n # Write settings dialog - # @return `:abort` if aborted and `:next` otherwise + # + # @return [Symbol] :abort if aborted + # :next otherwise def run Yast::Wizard.RestoreHelp(help_text) - ret = Yast::Bootloader.Write - ret ? :next : :abort + + Yast::Bootloader.Write ? :next : :abort end private diff --git a/src/modules/Bootloader.rb b/src/modules/Bootloader.rb index 17dac59aa..d085b4b64 100644 --- a/src/modules/Bootloader.rb +++ b/src/modules/Bootloader.rb @@ -227,56 +227,42 @@ def Write log.info "Writing bootloader configuration" - # run Progress bar stages = [ - # progress stage, text in dialog (short) _("Prepare system"), - # progress stage, text in dialog (short) _("Create initrd"), - # progress stage, text in dialog (short) _("Save boot loader configuration") ] titles = [ - # progress step, text in dialog (short) _("Preparing system..."), - # progress step, text in dialog (short) _("Creating initrd..."), - # progress step, text in dialog (short) _("Saving boot loader configuration...") ] - # progress bar caption + if Mode.normal - # progress line - Progress.New( - _("Saving Boot Loader Configuration"), - " ", - stages.size, - stages, - titles, - "" - ) + Progress.New(_("Saving Boot Loader Configuration"), " ", stages.size, stages, titles, "") Progress.NextStage else Progress.Title(titles[0]) end + # Prepare system progress_state = Progress.set(false) if !::Bootloader::BootloaderFactory.current.prepare - log.error "System could not be prepared successfully, required packages were not installed" + log.error("System could not be prepared successfully, required packages were not installed") Yast2::Popup.show(_("Cannot continue without install required packages")) return false end Progress.set(progress_state) + + # Create initrd Progress.NextStage Progress.Title(titles[1]) unless Mode.normal - ret = write_initrd - - log.error "Error occurred while creating initrd" unless ret + write_initrd || log.error("Error occurred while creating initrd") + # Save boot loader configuration Progress.NextStage Progress.Title(titles[2]) unless Mode.normal - ::Bootloader::BootloaderFactory.current.write true From 49bb7ec5587b6fae30725a858a73ca0097c0670a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 29 Aug 2018 09:22:36 +0100 Subject: [PATCH 4/6] Update version and changelog --- 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 994f871a5..5fb4bb0bb 100644 --- a/package/yast2-bootloader.changes +++ b/package/yast2-bootloader.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Aug 29 08:14:54 UTC 2018 - David Díaz + +- Do not crash when required package is not installed (bsc#1089829) +- 4.1.9 + ------------------------------------------------------------------- Wed Aug 22 16:33:37 CEST 2018 - schubi@suse.de diff --git a/package/yast2-bootloader.spec b/package/yast2-bootloader.spec index 9aea218c5..7e06f105e 100644 --- a/package/yast2-bootloader.spec +++ b/package/yast2-bootloader.spec @@ -17,7 +17,7 @@ Name: yast2-bootloader -Version: 4.1.8 +Version: 4.1.9 Release: 0 BuildRoot: %{_tmppath}/%{name}-%{version}-build From 0001c4c2cebfda8cd2cd2d6065a9e13b6ef923f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 29 Aug 2018 17:09:13 +0100 Subject: [PATCH 5/6] Fix typo --- src/lib/bootloader/bootloader_base.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib/bootloader/bootloader_base.rb b/src/lib/bootloader/bootloader_base.rb index 2ee68c056..c8a2720c8 100644 --- a/src/lib/bootloader/bootloader_base.rb +++ b/src/lib/bootloader/bootloader_base.rb @@ -99,7 +99,8 @@ def include_kexec_tools_package? # Writes the sysconfig readed in the initialization # - # Useful to "rollback" sysconfig changes if something is fails before finish the configuration + # Useful to "rollback" sysconfig changes if something fails before finish writing the + # configuration def restore_initial_sysconfig @initial_sysconfig.write end From a72a087ec81ccf48890d2a3dec4f9d26e4535e0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Thu, 30 Aug 2018 14:31:28 +0100 Subject: [PATCH 6/6] Fix the changelog Do not include the author's name --- package/yast2-bootloader.changes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/yast2-bootloader.changes b/package/yast2-bootloader.changes index 5fb4bb0bb..eb1d713b8 100644 --- a/package/yast2-bootloader.changes +++ b/package/yast2-bootloader.changes @@ -1,5 +1,5 @@ ------------------------------------------------------------------- -Wed Aug 29 08:14:54 UTC 2018 - David Díaz +Wed Aug 29 08:14:54 UTC 2018 - dgonzalez@suse.com - Do not crash when required package is not installed (bsc#1089829) - 4.1.9