From 6581a68bddb3cd6c0dcb2e1b890194c57d273c57 Mon Sep 17 00:00:00 2001 From: Ancor Gonzalez Sosa Date: Fri, 1 Dec 2017 17:57:31 +0100 Subject: [PATCH 1/2] Use the improved Device.compare_by_name from libstorage-ng --- src/lib/y2storage/blk_device.rb | 3 + src/lib/y2storage/comparable_by_name.rb | 80 ++++++++ src/lib/y2storage/device.rb | 19 ++ src/lib/y2storage/lvm_vg.rb | 3 + src/lib/y2storage/partition.rb | 18 -- src/lib/y2storage/partitionable.rb | 50 ----- .../devicegraphs/sorting/disks_and_dasds1.yml | 171 ++++++++++++++++++ .../devicegraphs/sorting/disks_and_dasds2.yml | 15 ++ test/y2storage/blk_device_test.rb | 30 +++ test/y2storage/devicegraph_test.rb | 2 +- test/y2storage/disk_test.rb | 6 +- test/y2storage/lvm_vg_test.rb | 7 + 12 files changed, 332 insertions(+), 72 deletions(-) create mode 100644 src/lib/y2storage/comparable_by_name.rb create mode 100644 test/data/devicegraphs/sorting/disks_and_dasds1.yml create mode 100644 test/data/devicegraphs/sorting/disks_and_dasds2.yml diff --git a/src/lib/y2storage/blk_device.rb b/src/lib/y2storage/blk_device.rb index 9995dd36e0..b8c549dffa 100644 --- a/src/lib/y2storage/blk_device.rb +++ b/src/lib/y2storage/blk_device.rb @@ -22,6 +22,7 @@ require "y2storage/storage_class_wrapper" require "y2storage/device" require "y2storage/hwinfo_reader" +require "y2storage/comparable_by_name" module Y2Storage # Base class for most devices having a device name, udev path and udev ids. @@ -31,6 +32,8 @@ class BlkDevice < Device wrap_class Storage::BlkDevice, downcast_to: ["Partitionable", "Partition", "Encryption", "LvmLv"] + include ComparableByName + # @!method self.all(devicegraph) # @param devicegraph [Devicegraph] # @return [Array] all the block devices in the given devicegraph diff --git a/src/lib/y2storage/comparable_by_name.rb b/src/lib/y2storage/comparable_by_name.rb new file mode 100644 index 0000000000..59430e5028 --- /dev/null +++ b/src/lib/y2storage/comparable_by_name.rb @@ -0,0 +1,80 @@ +# encoding: utf-8 + +# Copyright (c) [2017] SUSE LLC +# +# All Rights Reserved. +# +# This program is free software; you can redistribute it and/or modify it +# under the terms of version 2 of the GNU General Public License as published +# by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, contact SUSE LLC. +# +# To contact SUSE LLC about this file by physical or electronic mail, you may +# find current contact information at www.suse.com. + +require "y2storage/device" + +module Y2Storage + # Mixin for classes supporting to be sorted by name in libstorage-ng, offering + # methods to sort in a Ruby-friendly way and to query the whole sorted + # collection from a devicegraph. + # + # Classes including this mixin should be accepted by {Device.compare_by_name} + # (defined by libstorage-ng) and should implement the class method .all to + # query the whole collection from a devicegraph. + module ComparableByName + # Compare to another device by name, used for sorting sets of + # partitionable devices. + # + # @see Device.compare_by_name to check which types are accepted as argument + # (restricted by libstorage-ng). + # + # @raise [Storage::Exception] if trying to compare with something that is not + # supported by libstorage-ng. + # + # Using this method to compare and sort would result is something similar + # to alphabetical order but with some desired exceptions like: + # + # * /dev/sda, /dev/sdb, ..., /dev/sdaa + # * /dev/md1, /dev/md2, ..., /dev/md10 + # + # Unlike the class method {Device.compare_by_name}, which is boolean, + # this method follows the Ruby convention of returning -1 or 1 in the same + # cases than the <=> operator. + # + # @param other [BlkDevice, LvmVg] + # @return [Integer] -1 if this object should appear before the one passed as + # argument (less than). 1 otherwise. + def compare_by_name(other) + # In practice, two devices cannot have the same name. But let's take the + # case in consideration to ensure full compatibility with <=> + return 0 if name == other.name + Device.compare_by_name(self, other) ? -1 : 1 + end + + # Class methods for the mixin + module ClassMethods + # All the devices of the correspondig class found in the given devicegraph, + # sorted by name + # + # See {#compare_by_name} to know more about the sorting. + # + # @param devicegraph [Devicegraph] + # @return [Array<#compare_by_name>] + def sorted_by_name(devicegraph) + all(devicegraph).sort { |a, b| a.compare_by_name(b) } + end + end + + def self.included(base) + base.extend(ClassMethods) + end + end +end diff --git a/src/lib/y2storage/device.rb b/src/lib/y2storage/device.rb index 5b1683d90a..72dc13e23c 100644 --- a/src/lib/y2storage/device.rb +++ b/src/lib/y2storage/device.rb @@ -135,6 +135,25 @@ class Device storage_forward :userdata= protected :userdata, :userdata= + # @!method self.compare_by_name(lhs, rhs) + # Compare two devices by their name, used for sorting sets of + # block devices and/or LVM VGs. + # + # Using this method to compare and sort would result is something similar + # to alphabetical order but with some desired exceptions like: + # + # * /dev/sda, /dev/sdb, ..., /dev/sdaa + # * /dev/md1, /dev/md2, ..., /dev/md10 + # + # @raise [Storage::Exception] if trying to compare something that is not + # {BlkDevice} or {LvmVg} + # + # @param lhs [BlkDevice, LvmVg] + # @param rhs [BlkDevice, LvmVg] + # @return [boolean] true if the first argument should appear first in a + # sorted list (less than) + storage_class_forward :compare_by_name + # Ancestors in the devicegraph in no particular order, not including the # device itself. # diff --git a/src/lib/y2storage/lvm_vg.rb b/src/lib/y2storage/lvm_vg.rb index ae64d813f9..336b99ee3c 100644 --- a/src/lib/y2storage/lvm_vg.rb +++ b/src/lib/y2storage/lvm_vg.rb @@ -21,6 +21,7 @@ require "y2storage/storage_class_wrapper" require "y2storage/device" +require "y2storage/comparable_by_name" module Y2Storage # A Volume Group of the Logical Volume Manager (LVM) @@ -29,6 +30,8 @@ module Y2Storage class LvmVg < Device wrap_class Storage::LvmVg + include ComparableByName + # @!attribute vg_name # @return [String] volume group name (e.g."vg0"), not to be confused # with BlkDevice#name (e.g. "/dev/mapper/vg0") diff --git a/src/lib/y2storage/partition.rb b/src/lib/y2storage/partition.rb index b6bb6fd1e7..3c88675c00 100644 --- a/src/lib/y2storage/partition.rb +++ b/src/lib/y2storage/partition.rb @@ -134,24 +134,6 @@ def self.all(devicegraph) Partitionable.all(devicegraph).map(&:partitions).flatten end - # All partitions in the given devicegraph, sorted by name - # - # See {Partitionable#compare_by_name} to know more about the sorting. - # - # @param devicegraph [Devicegraph] - # @return [Array] - def self.sorted_by_name(devicegraph) - Partitionable.sorted_by_name(devicegraph).each_with_object([]) do |partitionable, result| - partitions = partitionable.partitions.sort do |a, b| - # Within the same partition table, the equivalent of Partitionable#compare_by_name - # is sorting by name size and then alphabetically - by_size = a.name.size <=> b.name.size - by_size.zero? ? a.name <=> b.name : by_size - end - result.concat(partitions) - end - end - # @return [String] def inspect "" diff --git a/src/lib/y2storage/partitionable.rb b/src/lib/y2storage/partitionable.rb index 1aaec4ab29..945b29baa5 100644 --- a/src/lib/y2storage/partitionable.rb +++ b/src/lib/y2storage/partitionable.rb @@ -83,56 +83,6 @@ class Partitionable < BlkDevice # in no particular order storage_class_forward :all, as: "Partitionable" - # @!method self.compare_by_name(lhs, rhs) - # Compare two devices by their name, used for sorting sets of - # partitionable devices. - # - # Using this method to compare and sort would result is something similar - # to alphabetical order but with some desired exceptions like: - # - # * /dev/sda, /dev/sdb, ..., /dev/sdaa - # * /dev/md1, /dev/md2, ..., /dev/md10 - # - # @param lhs [Partitionable] - # @param rhs [Partitionable] - # @return [boolean] true if the first argument should appear first in a - # sorted list (less than) - storage_class_forward :compare_by_name - - # Compare to another device by name, used for sorting sets of - # partitionable devices. - # - # Using this method to compare and sort would result is something similar - # to alphabetical order but with some desired exceptions like: - # - # * /dev/sda, /dev/sdb, ..., /dev/sdaa - # * /dev/md1, /dev/md2, ..., /dev/md10 - # - # Unlike the class method {Partitionable.compare_by_name}, which is boolean, - # this method follows the Ruby convention of returning -1 or 1 in the same - # cases than the <=> operator. - # - # @param other [Partitionable] - # @return [Integer] -1 if this object should appear before the one passed as - # argument (less than). 1 otherwise. - def compare_by_name(other) - # In practice, two devices cannot have the same name. But let's take the - # case in consideration to ensure full compatibility with <=> - return 0 if name == other.name - Partitionable.compare_by_name(self, other) ? -1 : 1 - end - - # All the devices of the correspondig class found in the given devicegraph, - # sorted by name - # - # See {Partitionable#compare_by_name} to know more about the sorting. - # - # @param devicegraph [Devicegraph] - # @return [Array<#compare_by_name>] - def self.sorted_by_name(devicegraph) - all(devicegraph).sort { |a, b| a.compare_by_name(b) } - end - # Partitions in the device # # @return [Array] diff --git a/test/data/devicegraphs/sorting/disks_and_dasds1.yml b/test/data/devicegraphs/sorting/disks_and_dasds1.yml new file mode 100644 index 0000000000..251f6a1c8c --- /dev/null +++ b/test/data/devicegraphs/sorting/disks_and_dasds1.yml @@ -0,0 +1,171 @@ +--- +# This file includes many holes in the names of the devices. The goal is not to +# represent a valid devicegraph but to present challenging combinations of names +# to test the stable sorting +- dasd: + name: /dev/dasda + size: 50 GiB + partition_table: dasd + partitions: + + - partition: + size: 1 GiB + name: /dev/dasda1 + + - partition: + size: 6 GiB + name: /dev/dasda2 + + - partition: + size: unlimited + name: /dev/dasda10 + +- disk: + name: /dev/sda + size: 50 GiB + +- dasd: + name: /dev/dasdab + size: 50 GiB + +- disk: + name: /dev/sdb + size: 800 GiB + partition_table: ms-dos + partitions: + + - partition: + size: 780 GiB + name: /dev/sdb1 + + - partition: + size: unlimited + name: /dev/sdb2 + +- dasd: + name: /dev/dasdb + size: 10 GiB + partition_table: dasd + partitions: + + - partition: + size: 1 GiB + name: /dev/dasdb1 + + - partition: + size: 6 GiB + name: /dev/dasdb2 + + - partition: + size: unlimited + name: /dev/dasdb3 + +- disk: + size: 800 GiB + name: "/dev/nvme0n2" + partition_table: gpt + partitions: + + - partition: + size: 75 GiB + name: /dev/nvme0n2p1 + + - partition: + size: 40 GiB + name: /dev/nvme0n2p2 + +- disk: + name: /dev/sdc + size: 500 GiB + partition_table: gpt + partitions: + + - partition: + size: 50 GiB + name: /dev/sdc1 + + - partition: + size: 2 GiB + name: /dev/sdc2 + + - partition: + size: 20 GiB + name: /dev/sdc3 + + - partition: + size: 20 GiB + name: /dev/sdc4 + + - partition: + size: 20 GiB + name: /dev/sdc10 + + - partition: + size: 20 GiB + name: /dev/sdc21 + +- disk: + size: 800 GiB + name: "/dev/nvme1n1" + partition_table: gpt + partitions: + + - partition: + size: 75 GiB + name: /dev/nvme1n1p2 + + - partition: + size: 40 GiB + name: /dev/nvme1n1p1 + +- disk: + name: /dev/sdaa + size: 500 GiB + partition_table: msdos + partitions: + + - partition: + size: 250 GiB + name: /dev/sdaa1 + + - partition: + size: 2 GiB + name: /dev/sdaa2 + + - partition: + size: 20 GiB + name: /dev/sdaa3 + +- disk: + size: 800 GiB + name: "/dev/nvme0n1" + partition_table: gpt + partitions: + + - partition: + size: 75 GiB + name: /dev/nvme0n1p1 + + - partition: + size: 40 GiB + name: /dev/nvme0n1p2 + + - partition: + size: 2 GiB + name: /dev/nvme0n1p3 + + - partition: + size: 2 GiB + name: /dev/nvme0n1p4 + + - partition: + size: 2 GiB + name: /dev/nvme0n1p10 + + - partition: + size: 2 GiB + name: /dev/nvme0n1p11 + + - partition: + size: 2 GiB + name: /dev/nvme0n1p40 diff --git a/test/data/devicegraphs/sorting/disks_and_dasds2.yml b/test/data/devicegraphs/sorting/disks_and_dasds2.yml new file mode 100644 index 0000000000..eba51a7fbb --- /dev/null +++ b/test/data/devicegraphs/sorting/disks_and_dasds2.yml @@ -0,0 +1,15 @@ +--- +# This file includes many holes in the names of the devices. The goal is not to +# represent a valid devicegraph but to present challenging combinations of names +# to test the stable sorting +- disk: + name: /dev/vda + size: 50 GiB + +- dasd: + name: /dev/vdaa + size: 50 GiB + +- disk: + name: /dev/vdb + size: 50 GiB diff --git a/test/y2storage/blk_device_test.rb b/test/y2storage/blk_device_test.rb index 8b89d5cabd..75ece8aef1 100644 --- a/test/y2storage/blk_device_test.rb +++ b/test/y2storage/blk_device_test.rb @@ -576,4 +576,34 @@ end end end + + describe ".sorted_by_name" do + let(:scenario) { "sorting/disks_and_dasds1" } + + it "returns all the blk devices sorted by name" do + devices = Y2Storage::BlkDevice.sorted_by_name(fake_devicegraph) + expect(devices.map(&:basename)).to eq %w( + dasda dasda1 dasda2 dasda10 dasdb dasdb1 dasdb2 dasdb3 dasdab + nvme0n1 nvme0n1p1 nvme0n1p2 nvme0n1p3 nvme0n1p4 nvme0n1p10 nvme0n1p11 nvme0n1p40 + nvme0n2 nvme0n2p1 nvme0n2p2 nvme1n1 nvme1n1p1 nvme1n1p2 + sda sdb sdb1 sdb2 sdc sdc1 sdc2 sdc3 sdc4 sdc10 sdc21 sdaa sdaa1 sdaa2 sdaa3 + ) + end + end + + describe "#compare_by_name" do + context "when devices of different types share a common name structure" do + let(:scenario) { "sorting/disks_and_dasds2" } + + let(:vda) { Y2Storage::Disk.find_by_name(fake_devicegraph, "/dev/vda") } + let(:vdaa) { Y2Storage::Dasd.find_by_name(fake_devicegraph, "/dev/vdaa") } + let(:vdb) { Y2Storage::Disk.find_by_name(fake_devicegraph, "/dev/vdb") } + + it "allows to sort them by name, no matter the input order" do + [vda, vdaa, vdb].permutation do |input| + expect(input.sort { |a, b| a.compare_by_name(b) }).to eq [vda, vdb, vdaa] + end + end + end + end end diff --git a/test/y2storage/devicegraph_test.rb b/test/y2storage/devicegraph_test.rb index 6bf5099c80..66fba628a5 100644 --- a/test/y2storage/devicegraph_test.rb +++ b/test/y2storage/devicegraph_test.rb @@ -267,7 +267,7 @@ def with_sda2_deleted(initial_graph) def less_than_next(device, collection) next_dev = collection[collection.index(device) + 1] - next_dev.nil? || Y2Storage::Partitionable.compare_by_name(device, next_dev) + next_dev.nil? || device.compare_by_name(next_dev) < 0 end context "if there are no multi-disk devices" do diff --git a/test/y2storage/disk_test.rb b/test/y2storage/disk_test.rb index 9db1f7fb08..cb2947e3db 100644 --- a/test/y2storage/disk_test.rb +++ b/test/y2storage/disk_test.rb @@ -361,7 +361,7 @@ def disk(disk_name) end describe ".sorted_by_name" do - let(:scenario) { "autoyast_drive_examples" } + let(:scenario) { "sorting/disks_and_dasds1" } it "returns a list of Y2Storage::Disk objects" do disks = Y2Storage::Disk.sorted_by_name(fake_devicegraph) @@ -372,7 +372,7 @@ def disk(disk_name) it "includes all disks in the devicegraph, sorted by name, and nothing else" do disks = Y2Storage::Disk.sorted_by_name(fake_devicegraph) expect(disks.map(&:basename)).to eq [ - "nvme0n1", "sda", "sdb", "sdc", "sdd", "sdf", "sdh", "sdaa" + "nvme0n1", "nvme0n2", "nvme1n1", "sda", "sdb", "sdc", "sdaa" ] end @@ -387,7 +387,7 @@ def disk(disk_name) it "returns an array sorted by name" do disks = Y2Storage::Disk.sorted_by_name(fake_devicegraph) expect(disks.map(&:basename)).to eq [ - "nvme0n1", "sda", "sdb", "sdc", "sdd", "sdf", "sdh", "sdaa" + "nvme0n1", "nvme0n2", "nvme1n1", "sda", "sdb", "sdc", "sdaa" ] end end diff --git a/test/y2storage/lvm_vg_test.rb b/test/y2storage/lvm_vg_test.rb index df6066cdc6..58fad7b7c8 100644 --- a/test/y2storage/lvm_vg_test.rb +++ b/test/y2storage/lvm_vg_test.rb @@ -35,4 +35,11 @@ expect(subject.name).to include(subject.vg_name) end end + + describe ".sorted_by_name" do + it "returns all the volume groups sorted by name" do + devices = Y2Storage::LvmVg.sorted_by_name(fake_devicegraph) + expect(devices.map(&:basename)).to eq ["vg0", "vg1"] + end + end end From ced68d19f191499260d6528595e266fca290f66a Mon Sep 17 00:00:00 2001 From: Ancor Gonzalez Sosa Date: Fri, 1 Dec 2017 18:00:17 +0100 Subject: [PATCH 2/2] Version and changelog --- package/yast2-storage-ng.changes | 8 ++++++++ package/yast2-storage-ng.spec | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/package/yast2-storage-ng.changes b/package/yast2-storage-ng.changes index 738d7bc43b..09ee49d3c7 100644 --- a/package/yast2-storage-ng.changes +++ b/package/yast2-storage-ng.changes @@ -1,3 +1,11 @@ +------------------------------------------------------------------- +Fri Dec 1 16:58:14 UTC 2017 - ancor@suse.com + +- Rely on the new improved mechanism of libstorage-ng to sort + devices by name. Preparation for bsc#1049901 and part of + fate#31896. +- 4.0.47 + ------------------------------------------------------------------- Fri Dec 1 15:34:11 UTC 2017 - igonzalezsosa@suse.com diff --git a/package/yast2-storage-ng.spec b/package/yast2-storage-ng.spec index edcd4f0cad..9a7a1915e6 100644 --- a/package/yast2-storage-ng.spec +++ b/package/yast2-storage-ng.spec @@ -16,7 +16,7 @@ # Name: yast2-storage-ng -Version: 4.0.46 +Version: 4.0.47 Release: 0 BuildArch: noarch