On Thu, 30 Mar 2017 18:53:42 +0200 Ancor Gonzalez Sosa <[email protected]> wrote:
> This is a follow-up on this discussion > https://github.com/yast/yast-storage-ng/pull/184#pullrequestreview-29672717 > > The mail is mainly targeted to Josef, so we continue the discussion > in a more persistent and prominent place than a merged pull request. > Don't feel guilty if you decide to ignore it. :-) Thanks for writing it. > > Before introducing the Y2Storage wrapper classes on top of the Storage > ones, we had a query API (already discussed in this list) based on the > concept of Y2Storage::DevicesLists. > > I think we don't need that anymore with the wrapper. I would go for an > approach based on pure Ruby arrays, now that we don't have to fear > not-found exceptions or to care about downcasting. I would simply add > the following convenience methods to Y2Storage::Device and that's it. > https://github.com/yast/yast-storage-ng/pull/184/commits/0f733850786242cddf79c7c3f8a733428d8c5674 Well, my biggest problem with this approach is that ancestor have to know about all its kids to have comprehensive API, which is very wrong approach from my POV. It also have to be modified, when new child is introduced to have it on same level as others. It should be responsibility of caller to know specific child if he want it. Device responsibility is to provide methods that are same for all devices and makes sense for them like comparing devices, its display_name or its ancestors. Consider if ruby use this approach and Object have methods like `string?` or `integer?`, do you feel it is good approach? Object have `is_a?` which moves responsibility to know about child to caller. So I suggest to use common ruby stuff. > > So, how would we query the devicegraph? Let's explain it by example. > Most examples are based on real cases I have seen while porting code > in other packages... others are there for the sake of the challenge ;) > > 1) The filesystem, encrypted or not, in a partition. That is, we want > the filesystem and we don't care whether there is an encryption (LUKS) > layer sitting between the partition and that filesystem. > > 2) The filesystem directly placed in a partition, nil if no filesystem > or if the filesystem is not directly there (e.g. there is an > encryption layer in between). > > 3) The encrypted filesystem in a partition, nil if the partition is > not formatted or if its directly formatted (without the intermediate > encryption layer) > > 4) The disk that match a disk name or partition name, only if it's GPT > > 5) Filesystems (encrypted or not) on logical partitions of a given > disk > > 6) Non-encrypted filesystem on logical partitions of a given disk > > 7) Logical partitions for the disk that match a disk name or contains > partition name. > > 8) From a given set of volume groups, physical volumes that are not > directly into a disk (or an encrypted disk) (e.g. they sit in a > partition or a RAID device) > > 9) Checking if a given partition contains a btrfs filesystem > (encrypted or not) > > 10) Disks hosting a given filesystem either directly or through one of > its partitions. "Disks" in plural because in our devicegraphs, a > filesystem can be lay across several block devices. > > 11) Disks "supporting" the existence of a given filesystem, for > whatever technology is used. For example, if the filesystem is in a > LV (LVM) and that LV sits on a VG, we want all the disks hosting the > PVs of that VG, either directly or through partitions or RAID devices > (yes, this is one of the examples based on real code). > > 12) Disks of a filesystem, only if the plain disk is directly > formatted (no partitions, RAID or encryption involved). Otherwise nil > > 13) Disks of a filesystem, only if the disk is directly formatted (no > partitions), nil otherwise, but supporting encrypted filesystems. > > 14) Devices with a extX filesystem (encrypted or not) > > 15) Partitions with a extX filesystem (encrypted or not) > > How would we do that just with the wrapper and plain arrays? > I write how it will look like, for examples where it makes difference ( I consider we are in Y2Storage namespace, otherwise it need to be prepended by it ): > 1) > the_partition.blk_filesystem > > 2) > the_partition.direct_blk_filesystem > > 3) > the_partition.encrypted? ? the_partition.blk_filesystem : nil > > 4) > disk = devgraph.disks.detect {|d| d.name_or_partition?(the_name) } > (disk && disk.gpt?) ? disk : nil > > # or > > disk = Y2Storage::Disk.find_by_name_or_partition(devgraph, the_name) > (disk && disk.gpt?) ? disk : nil > > 5) > parts = the_disk.partitions.select {|p| p.type.is?(:logical) } > parts.map {|p| p.blk_filesystem }.compact > > 6) > parts = the_disk.partitions.select {|p| p.type.is?(:logical) } > parts.map {|p| p.direct_blk_filesystem }.compact > > 7) > disk = devgraph.disks.detect {|d| d.name_or_partition?(the_name) } > disk.partitions.select {|p| p.type.is?(:logical) } > > 8) > all_pvs = the_vol_groups.map(&:lvm_pvs).flatten > all_pvs.reject { |pv| pv.blk_device.plain_device.disk? } > all_pvs = the_vol_groups.map(&:lvm_pvs).flatten all_pvs.reject { |pv| pv.blk_device.plain_device.is_a?(Disk) } > 9) > !!(partition.filesystem && partition.filesystem.type.is?(:btrfs)) > > 10) > # See notes below > devgraph.disks.select do |d| > d.filesystem == the_filesystem || > d.partitions.any? {|p| p.filesystem == the_filesystem } > end > > # Beware, this is not equivalent because it also returns filesystems > in # partitions of a software RAID or any other partitionable device > that # is not a disk > the_filesystem.ancestors.select do |a| > (a.disk? || a.partition?) && a.filesystem == the_filesystem > end the_filesystem.ancestors.select do |a| (a.is?(Disk) || a.is_a?(Partition) && a.filesystem == the_filesystem end > > 11) > the_filesystem.ancestors.select(&:disk?).uniq the_filesystem.ancestors.grep(Disk).uniq > > 12) > filesystem.blk_devices.select(&:disk?) filesystem.blk_devices.grep(Disk) > > 13) > filesystem.plain_blk_devices.select(&:disk?) > filesystem.plain_blk_devices.grep(Disk) > 14) > all_ext = devgraph.blk_filesystems.select {|fs| fs.type.is?(:ext) } > all_ext.map {|fs| fs.plain_blk_devices }.flatten > > 15) > devgraph.partitions.select do |p| > p.blk_filesystem && p.blk_filesystem.type.is?(:ext) > end > > Feel free to provide more "challenges" and/or test alternative API > against the current ones. My challenge for you what have to changed if we add new MD Raid? So how hard will be to add new child to Device. This is good metric for me how hard/easy is to extend it. Josef > > And yes, before you ask, it would make sense to add this to BlkDevice > alias_method :filesystem, :blk_filesystem > alias_method :direct_filesystem, :direct_blk_filesystem > > Moreover, if we find ourselves too often checking for something in a > disk and all its partitions, we could add a convenience method > Y2Storage::Partitionable#this_and_partitions. > Then some examples would change a bit, like: > > 10) > devgraph.disks.select do |d| > d.this_and_partitions.any? {|i| i.filesystem == the_filesystem } > end > > Looking forward alternatives and/or feedback. > > Cheers. -- To unsubscribe, e-mail: [email protected] To contact the owner, e-mail: [email protected]
