Merge pull request #528 from mchf/master

Do not crash with internal error when GRUB_TERMINAL contains multiple values
yast · Aug 20, 2018 · 22aa181 · 22aa181
2 parents b6c4a5e + dfb5752
commit 22aa181
Show file tree

Hide file tree

Showing 9 changed files with 106 additions and 58 deletions.
diff --git a/README.md b/README.md
@@ -47,3 +47,7 @@ that holds and also can propose the bootloader implementation. So now let's expl
 - [Boot Record backup](https://www.rubydoc.info/github/yast/yast-bootloader/master/Bootloader/BootRecordBackup) creates a backup of boot record for devices which the code touched
 - [Serial Console](https://www.rubydoc.info/github/yast/yast-bootloader/master/Bootloader/SerialConsole) converts between grub2 serial console configuration and kernel serial console configuration
 - [Sysconfig](https://www.rubydoc.info/github/yast/yast-bootloader/master/Bootloader/Sysconfig) read/write sysconfig configuration for bootloader. Sysconfig basically holds currently used grub2 implementation and configuration that is not in grub2 itself like if use secure boot or trusted grub
+
+### See Also
+
+The bootloader module uses CFA (config files API) module for GRUB2 - [See CFA for GRUB2 repository](https://github.com/config-files-api/config_files_api_grub2)
diff --git a/package/yast2-bootloader.changes b/package/yast2-bootloader.changes
@@ -1,3 +1,11 @@
+-------------------------------------------------------------------
+Wed Aug 15 07:41:01 UTC 2018 - mfilka@suse.com
+
+- bnc#1053559
+  - do not crash with internal error when GRUB_TERMINAL contains
+    multiple values
+- 4.1.7
+
 -------------------------------------------------------------------
 Fri Jul 20 15:12:13 CEST 2018 - schubi@suse.de
 

diff --git a/package/yast2-bootloader.spec b/package/yast2-bootloader.spec
@@ -17,7 +17,7 @@
 
 
 Name:           yast2-bootloader
-Version:        4.1.6
+Version:        4.1.7
 Release:        0
 
 BuildRoot:      %{_tmppath}/%{name}-%{version}-build
@@ -31,7 +31,7 @@ BuildRequires:  yast2-ruby-bindings >= 1.0.0
 BuildRequires:  yast2-storage-ng >= 4.0.90
 # lenses needed also for tests
 BuildRequires:  augeas-lenses
-BuildRequires:  rubygem(%rb_default_ruby_abi:cfa_grub2) >= 0.5.1
+BuildRequires:  rubygem(%rb_default_ruby_abi:cfa_grub2) >= 1.0.1
 BuildRequires:  rubygem(%rb_default_ruby_abi:rspec)
 BuildRequires:  rubygem(%rb_default_ruby_abi:yast-rake)
 PreReq:         /bin/sed %fillup_prereq
@@ -44,8 +44,8 @@ Requires:       yast2-packager >= 2.17.24
 Requires:       yast2-pkg-bindings >= 2.17.25
 # Y2Storage::Mountable#mount_path
 Requires:       yast2-storage-ng >= 4.0.90
-# GrubCfg with boot_entries that filter out unbootable entries
-Requires:       rubygem(%rb_default_ruby_abi:cfa_grub2) >= 0.5.1
+# Support for multiple values in GRUB_TERMINAL
+Requires:       rubygem(%rb_default_ruby_abi:cfa_grub2) >= 1.0.1
 # lenses are needed as cfa_grub2 depends only on augeas bindings, but also
 # lenses are needed here
 Requires:       augeas-lenses

diff --git a/src/lib/bootloader/autoyast_converter.rb b/src/lib/bootloader/autoyast_converter.rb
@@ -88,7 +88,14 @@ def import_default(data, default)
           val = data["global"][key]
           next unless val
 
-          default.public_send(:"#{method}=", SYMBOL_PARAM.include?(key) ? val.to_sym : val)
+          default.public_send(:"#{method}=", val)
+        end
+
+        DEFAULT_ARRAY_MAPPING.each do |key, method|
+          val = data["global"][key]
+          next unless val
+
+          default.public_send(:"#{method}=", val.split.map { |v| v.to_sym })
         end
 
         DEFAULT_KERNEL_PARAMS_MAPPING.each do |key, method|
@@ -208,8 +215,11 @@ def export_grub2(res, bootloader)
       }.freeze
 
       DEFAULT_STRING_MAPPING = {
-        "gfxmode"  => :gfxmode,
-        "serial"   => :serial_console,
+        "gfxmode" => :gfxmode,
+        "serial"  => :serial_console
+      }.freeze
+
+      DEFAULT_ARRAY_MAPPING = {
         "terminal" => :terminal
       }.freeze
 
@@ -219,9 +229,6 @@ def export_grub2(res, bootloader)
         "xen_kernel_append" => :xen_hypervisor_params
       }.freeze
 
-      SYMBOL_PARAM = [
-        "terminal"
-      ].freeze
       def export_default(res, default)
         DEFAULT_BOOLEAN_MAPPING.each do |key, method|
           val = default.public_send(method)
@@ -233,6 +240,11 @@ def export_default(res, default)
           res[key] = val.to_s if val
         end
 
+        DEFAULT_ARRAY_MAPPING.each do |key, method|
+          val = default.public_send(method)
+          res[key] = val.join(" ") if val
+        end
+
         DEFAULT_KERNEL_PARAMS_MAPPING.each do |key, method|
           val = default.public_send(method)
           res[key] = val.serialize unless val.empty?

diff --git a/src/lib/bootloader/grub2_widgets.rb b/src/lib/bootloader/grub2_widgets.rb
@@ -450,7 +450,14 @@ def help
     end
   end
 
-  # Represents graphical and serial console for bootloader
+  # Represents graphical and serial console setup for bootloader
+  #
+  # Allows to configure terminal for grub. It can configure grub
+  # to use either graphical terminal, console or console over serial line.
+  #
+  # Graphical or serial terminal has to be selected explicitly. Either
+  # one of them or both at once.
+  # Native console is configured as a fallback when nothing else is selected.
   class ConsoleWidget < CWM::CustomWidget
     include Grub2Widget
 
@@ -482,23 +489,10 @@ def help
     end
 
     def init
-      enable = grub_default.terminal == :serial
-      Yast::UI.ChangeWidget(Id(:console_frame), :Value, enable)
-      args = grub_default.serial_console || ""
-      Yast::UI.ChangeWidget(Id(:console_args), :Value, args)
-
-      enable = grub_default.terminal == :gfxterm
-      Yast::UI.ChangeWidget(Id(:gfxterm_frame), :Value, enable)
-
-      Yast::UI.ChangeWidget(Id(:gfxmode), :Items, vga_modes_items)
-      mode = grub_default.gfxmode
-
-      # there's mode specified, use it
-      Yast::UI.ChangeWidget(Id(:gfxmode), :Value, mode) if mode && mode != ""
+      init_console
+      init_gfxterm
 
       Yast::UI.ChangeWidget(Id(:theme), :Value, grub_default.theme || "")
-    # FIXME: just temporary workaround for terminal that does contain
-    # more complex string (bsc#1053559)
     rescue RuntimeError
       raise ::Bootloader::UnsupportedOption, "GRUB_TERMINAL"
     end
@@ -528,16 +522,16 @@ def validate
     def store
       use_serial = Yast::UI.QueryWidget(Id(:console_frame), :Value)
       use_gfxterm = Yast::UI.QueryWidget(Id(:gfxterm_frame), :Value)
+      use_console = !use_serial && !use_gfxterm
 
-      use_gfxterm = false if use_gfxterm && use_serial
+      grub_default.terminal = []
+      grub_default.terminal = [:gfxterm] if use_gfxterm
 
       if use_serial
         console_value = Yast::UI.QueryWidget(Id(:console_args), :Value)
         BootloaderFactory.current.enable_serial_console(console_value)
-      elsif use_gfxterm
-        grub_default.terminal = :gfxterm
-      else
-        grub_default.terminal = :console
+      elsif use_console
+        grub_default.terminal = [:console]
       end
 
       mode = Yast::UI.QueryWidget(Id(:gfxmode), :Value)
@@ -566,6 +560,26 @@ def handle(event)
 
   private
 
+    # Initializates serial console specific widgets
+    def init_console
+      enable = grub_default.terminal.include?(:serial) if grub_default.terminal
+      Yast::UI.ChangeWidget(Id(:console_frame), :Value, enable)
+      args = grub_default.serial_console || ""
+      Yast::UI.ChangeWidget(Id(:console_args), :Value, args)
+    end
+
+    # Initializates gfxterm specific widgets
+    def init_gfxterm
+      enable = grub_default.terminal.include?(:gfxterm) if grub_default.terminal
+      Yast::UI.ChangeWidget(Id(:gfxterm_frame), :Value, enable)
+
+      Yast::UI.ChangeWidget(Id(:gfxmode), :Items, vga_modes_items)
+      mode = grub_default.gfxmode
+
+      # there's mode specified, use it
+      Yast::UI.ChangeWidget(Id(:gfxmode), :Value, mode) if mode && mode != ""
+    end
+
     # Explanation for help and error messages
     def syntax
       # Translators: NUM is an abbreviation for "number",

diff --git a/src/lib/bootloader/grub2base.rb b/src/lib/bootloader/grub2base.rb
@@ -212,17 +212,17 @@ def merge_grub_default(other)
     end
 
     def merge_attributes(default, other)
-      begin
-        # string attributes
-        [:serial_console, :terminal, :timeout, :hidden_timeout, :distributor,
-         :gfxmode, :theme, :default].each do |attr|
-          val = other.public_send(attr)
-          default.public_send((attr.to_s + "=").to_sym, val) if val
-        end
-      # FIXME: only temporary solution to catch too complex grub terminal option (bsc#1053559)
-      # will be removed when cfa_grub2 and yast understand more complex terminal configuration
-      rescue RuntimeError
-        raise ::Bootloader::UnsupportedOption, "GRUB_TERMINAL"
+      # string attributes
+      [:serial_console, :timeout, :hidden_timeout, :distributor,
+       :gfxmode, :theme, :default].each do |attr|
+        val = other.public_send(attr)
+        default.public_send((attr.to_s + "=").to_sym, val) if val
+      end
+
+      # array attributes with multiple values allowed
+      [:terminal].each do |attr|
+        val = other.public_send(attr)
+        default.public_send((attr.to_s + "=").to_sym, val) if val
       end
 
       # specific attributes that are not part of cfa
@@ -266,7 +266,7 @@ def propose_terminal
 
       # for ppc: Boards with graphics are rare and those are PowerNV, where
       # modules are not used, see bsc#911682
-      grub_default.terminal = (Yast::Arch.s390 || Yast::Arch.ppc) ? :console : :gfxterm
+      grub_default.terminal = (Yast::Arch.s390 || Yast::Arch.ppc) ? [:console] : [:gfxterm]
       grub_default.generic_set("GRUB_GFXPAYLOAD_LINUX", "text") if Yast::Arch.ppc
     end
 

diff --git a/test/autoyast_converter_test.rb b/test/autoyast_converter_test.rb
@@ -60,7 +60,7 @@
       bootloader = subject.import("global" => data)
 
       expect(bootloader.grub_default.kernel_params.serialize).to eq "verbose nomodeset"
-      expect(bootloader.grub_default.terminal).to eq :gfxterm
+      expect(bootloader.grub_default.terminal).to eq [:gfxterm]
       expect(bootloader.grub_default.os_prober).to be_enabled
       expect(bootloader.grub_default.hidden_timeout).to eq "10"
       expect(bootloader.stage1).to be_activate
@@ -109,7 +109,7 @@
 
     it "export to global key configuration" do
       bootloader.grub_default.kernel_params.replace("verbose nomodeset")
-      bootloader.grub_default.terminal = :gfxterm
+      bootloader.grub_default.terminal = [:gfxterm]
       bootloader.grub_default.os_prober.enable
       bootloader.grub_default.hidden_timeout = "10"
       bootloader.stage1.activate = true

diff --git a/test/grub2_widgets_test.rb b/test/grub2_widgets_test.rb
@@ -519,7 +519,7 @@ def stub_widget_value(id, value)
     end
 
     it "checks serial console checkbox if grub use it" do
-      bootloader.grub_default.terminal = :serial
+      bootloader.grub_default.terminal = [:serial]
 
       expect(Yast::UI).to receive(:ChangeWidget).with(Id(:console_frame), :Value, true)
 
@@ -535,7 +535,7 @@ def stub_widget_value(id, value)
     end
 
     it "checks graphical console checkbox if grub use it" do
-      bootloader.grub_default.terminal = :gfxterm
+      bootloader.grub_default.terminal = [:gfxterm]
 
       expect(Yast::UI).to receive(:ChangeWidget).with(Id(:gfxterm_frame), :Value, true)
 
@@ -602,7 +602,7 @@ def stub_widget_value(id, value)
 
       subject.store
 
-      expect(bootloader.grub_default.terminal).to eq :serial
+      expect(bootloader.grub_default.terminal).to eq [:serial]
       expect(bootloader.grub_default.serial_console).to eq "serial --unit=1 --speed=9600 --parity=even"
       # it also sets console args to kernel params, but it will be duplication of serial console test
     end
@@ -612,18 +612,18 @@ def stub_widget_value(id, value)
 
       subject.store
 
-      expect(bootloader.grub_default.terminal).to eq :gfxterm
+      expect(bootloader.grub_default.terminal).to eq [:gfxterm]
     end
 
-    it "sets serial terminal if both graphical and serial is selected" do
+    it "sets terminal to enable both if graphical and serial is selected" do
       expect(Yast::UI).to receive(:QueryWidget).with(Id(:console_frame), :Value).and_return(true)
       expect(Yast::UI).to receive(:QueryWidget).with(Id(:gfxterm_frame), :Value).and_return(true)
       allow(Yast::UI).to receive(:QueryWidget).with(Id(:console_args), :Value)
         .and_return("serial --unit=1 --speed=9600 --parity=even")
 
       subject.store
 
-      expect(bootloader.grub_default.terminal).to eq :serial
+      expect(bootloader.grub_default.terminal).to eq [:gfxterm, :serial]
     end
 
     it "sets console terminal if neither graphical nor serial console selected" do
@@ -632,7 +632,7 @@ def stub_widget_value(id, value)
 
       subject.store
 
-      expect(bootloader.grub_default.terminal).to eq :console
+      expect(bootloader.grub_default.terminal).to eq [:console]
     end
 
     it "stores theme value" do

diff --git a/test/grub2base_test.rb b/test/grub2base_test.rb
@@ -144,7 +144,7 @@
           it "proposes to use serial terminal" do
             subject.propose
 
-            expect(subject.grub_default.terminal).to eq :serial
+            expect(subject.grub_default.terminal).to include :serial
           end
         end
 
@@ -156,7 +156,7 @@
           it "proposes to use console terminal" do
             subject.propose
 
-            expect(subject.grub_default.terminal).to eq :console
+            expect(subject.grub_default.terminal).to eq [:console]
           end
         end
       end
@@ -169,7 +169,7 @@
         it "proposes to use console terminal" do
           subject.propose
 
-          expect(subject.grub_default.terminal).to eq :console
+          expect(subject.grub_default.terminal).to eq [:console]
         end
 
         it "sets GFXPAYLOAD_LINUX to text" do
@@ -187,7 +187,7 @@
         it "proposes gfx terminal" do
           subject.propose
 
-          expect(subject.grub_default.terminal).to eq :gfxterm
+          expect(subject.grub_default.terminal).to eq [:gfxterm]
         end
       end
     end
@@ -426,15 +426,15 @@
       subject.grub_default.default = "0"
       other.grub_default.default = "saved"
 
-      subject.grub_default.terminal = :gfxterm
+      subject.grub_default.terminal = [:gfxterm]
 
       subject.grub_default.os_prober.enable
       other.grub_default.os_prober.disable
 
       subject.merge(other)
 
       expect(subject.grub_default.default).to eq "saved"
-      expect(subject.grub_default.terminal).to eq :gfxterm
+      expect(subject.grub_default.terminal).to eq [:gfxterm]
       expect(subject.grub_default.os_prober).to be_disabled
     end
 
@@ -455,6 +455,16 @@
       expect(subject.password).to be_password
     end
 
+    it "use terminal configuration specified in the merged object" do
+      TERMINAL_DEFINITION = [:console, :serial].freeze
+
+      allow(other.grub_default).to receive(:terminal).and_return(TERMINAL_DEFINITION)
+
+      subject.merge(other)
+
+      expect(subject.grub_default.terminal).to eql TERMINAL_DEFINITION
+    end
+
     it "overwrites default section with merged one if specified" do
       allow(other.sections).to receive(:all).and_return(["Win crap", "openSUSE"])
       allow(subject.sections).to receive(:all).and_return(["Win crap", "openSUSE"])