Fix calculation of udev mapping (bsc#1166096)

package/yast2-bootloader.spec

@@ -29,8 +29,8 @@ Source0:        %{name}-%{version}.tar.bz2
 BuildRequires:  yast2 >= 3.1.176
 BuildRequires:  yast2-devtools >= 4.2.2
 BuildRequires:  yast2-ruby-bindings >= 1.0.0
-# Y2Storage::Mountable#preferred_mount_by
-BuildRequires:  yast2-storage-ng >= 4.2.90
+# Y2Storage::BlkDevice#preferred_name
+BuildRequires:  yast2-storage-ng >= 4.2.102
 # lenses needed also for tests
 BuildRequires:  augeas-lenses
 BuildRequires:  rubygem(%rb_default_ruby_abi:cfa_grub2) >= 1.0.1
@@ -46,8 +46,8 @@ Requires:       yast2 >= 3.1.176
 Requires:       yast2-core >= 2.18.7
 Requires:       yast2-packager >= 2.17.24
 Requires:       yast2-pkg-bindings >= 2.17.25
-# Y2Storage::Mountable#preferred_mount_by
-Requires:       yast2-storage-ng >= 4.2.90
+# Y2Storage::BlkDevice#preferred_name
+Requires:       yast2-storage-ng >= 4.2.102
 # 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

src/lib/bootloader/udev_mapping.rb

@@ -91,22 +91,37 @@ def kernel_to_udev(dev)
         return dev
       end
 
-      udev = mount_by_udev(device) || device.name
+      udev = udev_path(device)
       log.info "udev device for #{dev.inspect} is #{udev.inspect}"
 
       udev
     end
 
-    # @return [String, nil] nil if the udev name cannot be found
-    def mount_by_udev(device)
+    # Most convenient udev name of the device
+    #
+    # If possible, the udev path is chosen based on the mount_by attribute of
+    # the filesystem. If the device is not mounted or there is no path for
+    # the specified mount_by, a preferred (and available) name is calculated.
+    #
+    # @param device [Y2Storage::BlkDevice]
+    # @return [String]
+    def udev_path(device)
       filesystem = device.filesystem
-      return nil unless filesystem
 
-      # If the device is not mounted, a preferred mount by option is calculated.
-      mount_by = filesystem.mount_by || filesystem.preferred_mount_by
-      return nil unless mount_by
-
-      device.path_for_mount_by(mount_by)
+      if filesystem
+        mount_by_name = filesystem.mount_by_name
+
+        if mount_by_name
+          log.info "udev_path: using the udev name of the configured mount_by"
+          mount_by_name
+        else
+          log.info "udev_path: using the preferred udev name for the filesystem"
+          filesystem.preferred_name
+        end
+      else
+        log.info "udev_path: not formatted, using preferred udev name for the block device"
+        device.preferred_name
+      end
     end
   end
 end

test/udev_mapping_test.rb

@@ -45,66 +45,59 @@
     before do
       # find by name creates always new instance, so to make mocking easier, mock it to return always same instance
       allow(Y2Storage::BlkDevice).to receive(:find_by_name).and_return(device)
-
-      allow(device).to receive(:path_for_mount_by).with(mount_by).and_return(udev_name)
     end
 
     let(:device) { find_device("/dev/sda3") }
 
-    let(:mount_by) { Y2Storage::Filesystems::MountByType.new(mount_by_option) }
-
-    context "when the device is mounted" do
+    context "when the device is formatted" do
       before do
-        device.filesystem.mount_point.mount_by = mount_by
+        # The libstorage-ng bindings constantly create new instances of the storage objects.
+        # To make mocking easier, mock it to return always the same instances
+        allow(device).to receive(:filesystem).and_return(device.filesystem)
       end
 
-      let(:mount_by_option) { :label }
-
-      context "and the udev name is available for the mount by option in the mount point" do
-        let(:udev_name) { "/dev/disk/by-label/test" }
-
-        it "returns the udev name according to the mount by option in the mount point" do
-          expect(subject.to_mountby_device(device.name)).to eq(udev_name)
+      context "and mounted (or marked to be mounted)" do
+        before do
+          allow(device.filesystem).to receive(:mount_by_name).and_return mount_by_name
         end
-      end
 
-      context "and the udev name is not available for the mount by option in the mount point" do
-        let(:udev_name) { nil }
+        context "and the udev name is available for the mount_by option in the mount point" do
+          let(:mount_by_name) { "/dev/something" }
 
-        # This is likely not the right fallback, it should use the preferred mount_by.
-        # And, by definition, the preferred mount_by is always available
-        it "returns the kernel name as fallback" do
-          expect(subject.to_mountby_device(device.name)).to eq("/dev/sda3")
+          it "returns the udev name according to the mount_by option in the mount point" do
+            expect(subject.to_mountby_device(device.name)).to eq mount_by_name
+          end
         end
-      end
-    end
 
-    context "when the device is not mounted" do
-      before do
-        device.filesystem&.remove_mount_point
+        context "and the udev name is not available for the mount by option in the mount point" do
+          let(:mount_by_name) { nil }
+
+          it "returns as fallback the name preferred by storage-ng" do
+            expect(device.filesystem).to receive(:preferred_name).and_return "/dev/preferred/fs"
 
-        allow_any_instance_of(Y2Storage::MountPoint).to receive(:preferred_mount_by)
-          .and_return(mount_by)
+            expect(subject.to_mountby_device(device.name)).to eq "/dev/preferred/fs"
+          end
+        end
       end
 
-      let(:mount_by_option) { :label }
+      context "but not mounted" do
+        before { device.filesystem&.remove_mount_point }
 
-      context "and the udev name is available for the preferred mount by option" do
-        let(:udev_name) { "/dev/disk/by-label/test" }
+        it "returns the preferred udev name for the filesystem" do
+          expect(device.filesystem).to receive(:preferred_name).and_return "/dev/preferred/fs"
 
-        it "returns the udev name according to the preferred mount by option" do
-          expect(subject.to_mountby_device(device.name)).to eq(udev_name)
+          expect(subject.to_mountby_device(device.name)).to eq "/dev/preferred/fs"
         end
       end
+    end
 
-      context "and the udev name is not available for the preferred mount by option" do
-        let(:udev_name) { nil }
+    context "when the device is not formatted" do
+      before { device.remove_descendants }
 
-        # This fallback is nice to have as extra check, but in general makes no sense.
-        # The preferred mount_by should always be available by definition
-        it "returns the kernel name as fallback" do
-          expect(subject.to_mountby_device(device.name)).to eq("/dev/sda3")
-        end
+      it "returns the preferred udev name for the block device" do
+        expect(device).to receive(:preferred_name).and_return "/dev/preferred/blk_dev"
+
+        expect(subject.to_mountby_device(device.name)).to eq "/dev/preferred/blk_dev"
       end
     end
 
@@ -120,7 +113,6 @@
 
       # These mocks are not needed
       let(:device) { nil }
-      let(:udev_name) { nil }
       let(:mount_by) { nil }
 
       it "returns an udev link if there is any available" do