Merge pull request #536 from yast/feature/fix-bsc-1089829

Avoid to crash when a required package is not installed

package/yast2-bootloader.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+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
+
 -------------------------------------------------------------------
 Wed Aug 22 16:33:37 CEST 2018 - schubi@suse.de
 

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

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
@@ -50,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
@@ -74,5 +87,22 @@ 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 fails before finish writing the
+    # configuration
+    def restore_initial_sysconfig
+      @initial_sysconfig.write
+    end
   end
 end

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

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

src/modules/Bootloader.rb

@@ -16,6 +16,7 @@
 # $Id$
 #
 require "yast"
+require "yast2/popup"
 require "bootloader/exceptions"
 require "bootloader/sysconfig"
 require "bootloader/bootloader_factory"
@@ -226,42 +227,42 @@ def Write
 
       log.info "Writing bootloader configuration"
 
-      # run Progress bar
       stages = [
-        # progress stage, text in dialog (short)
+        _("Prepare system"),
         _("Create initrd"),
-        # progress stage, text in dialog (short)
         _("Save boot loader configuration")
       ]
       titles = [
-        # progress step, text in dialog (short)
+        _("Preparing system..."),
         _("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
 
-      ret = write_initrd
-
-      log.error "Error occurred while creating initrd" unless ret
+      # 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")
+        Yast2::Popup.show(_("Cannot continue without install required packages"))
+        return false
+      end
+      Progress.set(progress_state)
 
-      Progress.NextStep
+      # Create initrd
+      Progress.NextStage
       Progress.Title(titles[1]) unless Mode.normal
 
+      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

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

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

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