From 85413b5c338ed3f8f55268a5f77441293be09663 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Wed, 4 Dec 2019 12:03:32 +0000 Subject: [PATCH 1/6] Add FilesystemReader class - This replaces the old ExistingFilesystem class --- src/lib/y2storage.rb | 4 +- src/lib/y2storage/existing_filesystem.rb | 247 -------------- src/lib/y2storage/filesystem_reader.rb | 282 ++++++++++++++++ test/y2storage/existing_filesystem_test.rb | 332 ------------------- test/y2storage/filesystem_reader_test.rb | 362 +++++++++++++++++++++ 5 files changed, 646 insertions(+), 581 deletions(-) delete mode 100644 src/lib/y2storage/existing_filesystem.rb create mode 100644 src/lib/y2storage/filesystem_reader.rb delete mode 100755 test/y2storage/existing_filesystem_test.rb create mode 100755 test/y2storage/filesystem_reader_test.rb diff --git a/src/lib/y2storage.rb b/src/lib/y2storage.rb index f77a53a331..cbb0dd0f48 100644 --- a/src/lib/y2storage.rb +++ b/src/lib/y2storage.rb @@ -1,4 +1,4 @@ -# Copyright (c) [2016-2017] SUSE LLC +# Copyright (c) [2016-2019] SUSE LLC # # All Rights Reserved. # @@ -64,7 +64,7 @@ require "y2storage/exceptions" require "y2storage/boot_requirements_checker" require "y2storage/disk_analyzer" -require "y2storage/existing_filesystem" +require "y2storage/filesystem_reader" require "y2storage/disk_size" require "y2storage/fake_device_factory" require "y2storage/free_disk_space" diff --git a/src/lib/y2storage/existing_filesystem.rb b/src/lib/y2storage/existing_filesystem.rb deleted file mode 100644 index 806bd7883b..0000000000 --- a/src/lib/y2storage/existing_filesystem.rb +++ /dev/null @@ -1,247 +0,0 @@ -# Copyright (c) [2015] 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 "yast" -require "fileutils" -require "yast2/execute" -require "y2storage/fstab" - -Yast.import "OSRelease" - -module Y2Storage - # Class representing a filesystem in the system and providing - # convenience methods to inspect its content - class ExistingFilesystem - include Yast::Logger - - # @return [Filesystems::Base] - attr_reader :filesystem - - # Constructor - # - # @param filesystem [Filesystems::Base] - # @param mount_point [String] - def initialize(filesystem, mount_point = "/mnt") - @filesystem = filesystem - @mount_point = mount_point - - @processed = false - end - - # Device to which the filesystem belongs to - # - # @return [BlkDevice] - def device - filesystem.blk_devices.first - end - - # Reads the release name from the filesystem - # - # @return [String, nil] nil if the release name cannot be read - def release_name - set_attributes unless processed? - @release_name - end - - # Reads the fstab file from the filesystem - # - # @return [Fstab, nil] nil if the fstab file cannot be read - def fstab - set_attributes unless processed? - @fstab - end - - # Reads the crypttab file from the filesystem - # - # @return [Crypttab, nil] nil if the crypttab file cannot be read - def crypttab - set_attributes unless processed? - @crypttab - end - - # Whether the filesystem contains the Raspberry Pi boot code in - # the root path - # - # @return [Boolean] - def rpi_boot? - set_attributes unless processed? - !!@rpi_boot - end - - # Whether the filesystem contains a MS Windows system - # - # @return [Boolean] - def windows? - set_attributes unless processed? - @windows - end - - protected - - # @return [Boolean] if the filesystem was already mounted to read all the relevant info - attr_reader :processed - alias_method :processed?, :processed - - # Sets attributes depending on the kind of system it contains (Windows or Linux) - def set_attributes - @windows = windows_filesystem? - - read_filesystem unless @windows - - @processed = true - end - - # Whether the filesystem contains a Windows system - # - # @return [Boolean] - def windows_filesystem? - return false if !windows_architecture? || !windows_partition? - - filesystem.detect_content_info.windows? - rescue Storage::Exception - log.warn("#{device.name} content info cannot be detected") - false - end - - # Whether the architecture of the system is supported by MS Windows - # - # @return [Boolean] - def windows_architecture? - # Should we include ARM here? - Yast::Arch.x86_64 || Yast::Arch.i386 - end - - # Whether the filesystem is created over a Windows-suitable partition - # - # @return [Boolean] - def windows_partition? - device.is?(:partition) && device.suitable_for_windows? - end - - # Reads needed info from the filesystem - def read_filesystem - mount - @release_name = read_release_name - @fstab = read_fstab - @crypttab = read_crypttab - @rpi_boot = check_rpi_boot - umount - rescue RuntimeError => e # FIXME: rescue ::Storage::Exception when SWIG bindings are fixed - log.error("CAUGHT exception: #{e} for #{device.name}") - nil - end - - # Mounts the device - # - # @see execute - # - # @note libstorage-ng has a couple of ways for immediate mounting/unmounting devices, but - # they cannot be easily used here. - # - # For example, BlkFilesystem::detect_content_info uses an internal EnsureMounted object. - # EnsureMounted mounts a given filesystem during its construction and unmounts it during - # its destuction. In ruby there is no a clear way of calling the destructor of a binding - # object, so EnsureMounted cannot be used for a temporary mount to inspect the filesystem - # content from YaST and then unmount it. - # - # Besides that, MountPoint offers MountPoint::immediate_activate and ::immediate_deactivate, - # but these methods only can be used with probed mount points. Internally, these methods - # use Mountable::Impl::immediate_activate and ::immediate_deactivate. Such methdos could - # be offered in the public API, but they require to create a temporary mount point for the - # filesystem to mount. Creating a mount point could have some implications, see - # {Device#update_etc_status}, and moreover, a possible existing mount point should be - # correctly restored. - # - # The library API needs to be extended to easily mount/umount a device in an arbitrary - # path without modifying the device (i.e., without changing its current mount point). - # - # @raise [RuntimeError] when the device cannot be mounted - def mount - cmd = ["/usr/bin/mount", "-o", "ro", device.name, @mount_point] - - raise "mount failed for #{device.name}" unless execute(*cmd) - end - - # Unmounts the device - # - # @see mount - # - # @raise [RuntimeError] when the device cannot be unmounted - def umount - cmd = ["/usr/bin/umount", "-R", @mount_point] - - raise "umount failed for #{@mount_point}" unless execute(*cmd) - end - - # Tries to read the release name - # - # @return [String, nil] nil if the filesystem does not contain a release name - def read_release_name - release_name = Yast::OSRelease.ReleaseName(@mount_point) - release_name.empty? ? nil : release_name - end - - # Tries to read a fstab file - # - # @return [Fstab, nil] nil if the filesystem does not contain a fstab file - def read_fstab - fstab_path = File.join(@mount_point, "etc", "fstab") - return nil unless File.exist?(fstab_path) - - Fstab.new(fstab_path, filesystem) - end - - # Tries to read a crypttab file - # - # @return [Crypttab, nil] nil if the filesystem does not contain a crypttab file - def read_crypttab - crypttab_path = File.join(@mount_point, "etc", "crypttab") - return nil unless File.exist?(crypttab_path) - - Crypttab.new(crypttab_path, filesystem) - end - - # Checks whether the Raspberry Pi boot code is in the root of the - # filesystem - # - # @return [Boolean] - def check_rpi_boot - # Only lower-case is expected, but since casing is usually tricky in FAT - # filesystem, let's do a second check just in case - ["bootcode.bin", "BOOTCODE.BIN"].each do |name| - path = File.join(@mount_point, name) - return true if File.exist?(path) - end - - false - end - - # Executes a given command - # - # For possible parameters, see Yast::Execute.locally!. - # - # @return [Boolean] true if the command finishes correctly; false otherwise. - def execute(*args) - Yast::Execute.locally!(*args) - true - rescue Cheetah::ExecutionFailed - false - end - end -end diff --git a/src/lib/y2storage/filesystem_reader.rb b/src/lib/y2storage/filesystem_reader.rb new file mode 100644 index 0000000000..99d2474962 --- /dev/null +++ b/src/lib/y2storage/filesystem_reader.rb @@ -0,0 +1,282 @@ +# Copyright (c) [2019] 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 "yast" +require "fileutils" +require "yast2/execute" + +Yast.import "OSRelease" + +module Y2Storage + # Class to read the content of a filesystem + # + # The filesystem is mounted only the first time that any info is requested + class FilesystemReader + include Yast::Logger + + # Constructor + # + # @param filesystem [Filesystems::Base] + # @param mount_point [String] + def initialize(filesystem, mount_point = "/mnt") + @filesystem = filesystem + @mount_point = mount_point + end + + # Whether the filesystem contains a Windows system + # + # @return [Boolean] + def windows? + !!fs_attribute(:windows) + end + + # Linux release name from the filesystem + # + # @return [String, nil] nil if the release name is not found + def release_name + fs_attribute(:release_name) + end + + # Whether the filesystem contains the Raspberry Pi boot code in the root path + # + # @return [Boolean] + def rpi_boot? + !!fs_attribute(:rpi_boot) + end + + # Fstab raw content from the filesystem + # + # @return [String, nil] nil if the fstab file cannot be read + def fstab + fs_attribute(:fstab) + end + + # Crypttab raw content from the filesystem + # + # @return [Crypttab, nil] nil if the crypttab file cannot be read + def crypttab + fs_attribute(:crypttab) + end + + private + + # @return [Filesystems::Base] + attr_reader :filesystem + + # @return [String] + attr_reader :mount_point + + # Attributes that are read from the filesystem + FS_ATTRIBUTES = { + windows: nil, + release_name: nil, + rpi_boot: nil, + fstab: nil, + crypttab: nil + }.freeze + + # Filesystem attributes + # + # @return [Hash] + def fs_attributes + @fs_attributes ||= FS_ATTRIBUTES.dup + end + + # A filesystem attribute + # + # Note that the filesystem is mounted the first time that an attribute is requested. + # + # @param attr [Symbol] :windows, :release_name, :rpi_boot, :fstab, :crypttab + # @return [Object] + def fs_attribute(attr) + read unless @already_read + + fs_attributes[attr] + end + + # Save the value of a filesyste attribute + # + # @param attr [Symbol] + # @param value [Object] + def save_fs_attribute(attr, value) + fs_attributes[attr] = value + end + + # Reads the filesystem attributes + # + # Note that the filesystem is mounted the first time that the attributes are read. + def read + @already_read = true + + windows_system? ? read_windows_system : read_linux_system + end + + # Reads attributes for a Windows system + def read_windows_system + save_fs_attribute(:windows, true) + end + + # Reads attributes for a Linux system + def read_linux_system + save_fs_attribute(:windows, false) + + mount + save_fs_attribute(:release_name, read_release_name) + save_fs_attribute(:rpi_boot, check_rpi_boot) + save_fs_attribute(:fstab, read_fstab) + save_fs_attribute(:crypttab, read_crypttab) + umount + rescue RuntimeError => e # FIXME: rescue ::Storage::Exception when SWIG bindings are fixed + log.error("CAUGHT exception: #{e}") + nil + end + + # Whether the filesystem contains a Windows system + # + # Note that the filesystem needs to be mounted, see {Filesystems::BlkFilesystem#detect_content_info}. + # + # @return [Boolean] + def windows_system? + return false unless filesystem.windows_suitable? + + filesystem.detect_content_info.windows? + rescue Storage::Exception + log.warn("content info cannot be detected for filesystem #{filesystem.uuid}") + false + end + + # Reads the Linux release name + # + # @return [String, nil] nil if the filesystem does not contain a release name + def read_release_name + # This check is needed because {Yast::OSRelease.ReleaseName} returns a default release name when + # the file is not found. + release_file_path = File.join(mount_point, Yast::OSRelease.class::OS_RELEASE_PATH) + return nil unless File.exist?(release_file_path) + + release_name = Yast::OSRelease.ReleaseName(mount_point) + + release_name.empty? ? nil : release_name + end + + # Reads the fstab file + # + # @return [String, nil] nil if the filesystem does not contain a fstab file + def read_fstab + read_etc_file("fstab") + end + + # Reads the crypttab file + # + # @return [String, nil] nil if the filesystem does not contain a crypttab file + def read_crypttab + read_etc_file("crypttab") + end + + # Reads an etc file (fstab or crypttab) + # + # @param file_name [String] "etc", "crypttab" + # @return [String, nil] nil if the filesystem does not contain that etc file + def read_etc_file(file_name) + path = File.join(mount_point, "etc", file_name) + return nil unless File.exist?(path) + + File.readlines(path).join + end + + # Checks whether the Raspberry Pi boot code is in the root of the filesystem + # + # @return [Boolean] + def check_rpi_boot + # Only lower-case is expected, but since casing is usually tricky in FAT + # filesystem, let's do a second check just in case + ["bootcode.bin", "BOOTCODE.BIN"].each do |name| + path = File.join(mount_point, name) + return true if File.exist?(path) + end + + false + end + + # Mounts the filesystem + # + # @see execute + # + # @note libstorage-ng has a couple of ways for immediate mounting/unmounting devices, but + # they cannot be easily used here. + # + # For example, BlkFilesystem::detect_content_info uses an internal EnsureMounted object. + # EnsureMounted mounts a given filesystem during its construction and unmounts it during + # its destuction. In ruby there is no a clear way of calling the destructor of a binding + # object, so EnsureMounted cannot be used for a temporary mount to inspect the filesystem + # content from YaST and then unmount it. + # + # Besides that, MountPoint offers MountPoint::immediate_activate and ::immediate_deactivate, + # but these methods only can be used with probed mount points. Internally, these methods + # use Mountable::Impl::immediate_activate and ::immediate_deactivate. Such methdos could + # be offered in the public API, but they require to create a temporary mount point for the + # filesystem to mount. Creating a mount point could have some implications, see + # {Device#update_etc_status}, and moreover, a possible existing mount point should be + # correctly restored. + # + # The library API needs to be extended to easily mount/umount a device in an arbitrary + # path without modifying the device (i.e., without changing its current mount point). + # + # @raise [RuntimeError] when the filesystem cannot be mounted + def mount + cmd = ["/usr/bin/mount", "-o", "ro", mount_name, mount_point] + + raise "mount failed for #{mount_name}" unless execute(*cmd) + end + + # Unmounts the filesystem + # + # @see mount + # + # @raise [RuntimeError] when the filesystem cannot be unmounted + def umount + cmd = ["/usr/bin/umount", "-R", mount_point] + + raise "umount failed for #{mount_point}" unless execute(*cmd) + end + + # Device name to use when mounting a filesystem + # + # Note that the filesystem must exist on disk, so it should have an UUID. + # + # @return [String] + def mount_name + return filesystem.name if filesystem.is?(:nfs) + + "UUID=#{filesystem.uuid}" + end + + # Executes a given command + # + # For possible parameters, see Yast::Execute.locally!. + # + # @return [Boolean] true if the command finishes correctly; false otherwise. + def execute(*args) + Yast::Execute.locally!(*args) + true + rescue Cheetah::ExecutionFailed + false + end + end +end diff --git a/test/y2storage/existing_filesystem_test.rb b/test/y2storage/existing_filesystem_test.rb deleted file mode 100755 index aacfa81079..0000000000 --- a/test/y2storage/existing_filesystem_test.rb +++ /dev/null @@ -1,332 +0,0 @@ -#!/usr/bin/env rspec -# Copyright (c) [2016-2019] 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_relative "spec_helper" -require "y2storage" - -RSpec.shared_examples "Mount and umount actions" do - it "mounts the device" do - expect(Yast::Execute).to receive(:locally!).with(*mount_cmd) - subject.send(tested_method) - end - - it "umounts the device" do - expect(Yast::Execute).to receive(:locally!).with(*umount_cmd) - subject.send(tested_method) - end - - let(:cheetah_error) { Cheetah::ExecutionFailed.new([], "", nil, nil) } - - context "when mount fails" do - before do - allow(Yast::Execute).to receive(:locally!).with(*mount_cmd) - .and_raise(cheetah_error) - end - - it "does not perform the corresponding action" do - expect(subject).to_not receive("read_#{tested_method}") - subject.send(tested_method) - end - - it "returns nil (or false for boolean methods)" do - expect(subject.send(tested_method)).to eq result_if_mount_fails - end - end - - context "when umount fails" do - before do - allow(Yast::Execute).to receive(:locally!).with(*umount_cmd) - .and_raise(cheetah_error) - end - - it "sets the value correctly" do - expect(subject.send(tested_method)).to_not be_nil - end - end -end - -describe Y2Storage::ExistingFilesystem do - before do - fake_scenario(scenario) - - allow(Yast::Execute).to receive(:locally!) - allow(filesystem).to receive(:detect_content_info).and_return(content_info) - end - - subject { described_class.new(filesystem, mount_point) } - - let(:mount_point) { "" } - - let(:mount_cmd) { ["/usr/bin/mount", "-o", "ro", device.name, mount_point] } - - let(:umount_cmd) { ["/usr/bin/umount", "-R", mount_point] } - - let(:result_if_mount_fails) { nil } - - let(:device) { fake_devicegraph.find_by_name(device_name) } - - let(:filesystem) { device.filesystem } - - let(:content_info) { instance_double(Storage::ContentInfo, windows?: windows_content) } - - let(:windows_content) { false } - - let(:scenario) { "windows-linux-free-pc" } - - let(:device_name) { "/dev/sda3" } - - describe "#device" do - it "returns the device of the filesystem" do - expect(subject.device).to eq(device) - end - end - - describe "#release_name" do - context "when the filesystem contains a Windows system" do - let(:device_name) { "/dev/sda1" } - - let(:windows_content) { true } - let(:architecture) { :x86_64 } - - it "returns nil" do - expect(subject.release_name).to be_nil - end - end - - context "when the filesystem does not contain a Windows system" do - let(:device_name) { "/dev/sda3" } - - before do - allow(Yast::OSRelease).to receive(:ReleaseName).and_return release_name - end - - let(:release_name) { "Open SUSE" } - - let(:tested_method) { :release_name } - - include_examples "Mount and umount actions" - - context "when there is an installed system" do - let(:release_name) { "Open SUSE" } - - it "returns the release name" do - expect(subject.release_name).to eq(release_name) - end - end - - context "when there is not an installed system" do - let(:release_name) { "" } - - it "returns nil" do - expect(subject.release_name).to be_nil - end - end - end - end - - describe "#fstab" do - context "when the filesystem contains a Windows system" do - let(:device_name) { "/dev/sda1" } - - let(:windows_content) { true } - let(:architecture) { :x86_64 } - - it "returns nil" do - expect(subject.fstab).to be_nil - end - end - - context "when the filesystem does not contain a Windows system" do - let(:device_name) { "/dev/sda3" } - - let(:tested_method) { :fstab } - - before do - allow(File).to receive(:exist?).and_return(exists_fstab) - end - - let(:exists_fstab) { true } - - include_examples "Mount and umount actions" - - context "when the fstab file does not exist" do - let(:exists_fstab) { false } - - it "returns nil" do - expect(subject.fstab).to be_nil - end - end - - context "when the fstab file exists" do - let(:exists_fstab) { true } - - it "returns the fstab" do - expect(subject.fstab).to be_a(Y2Storage::Fstab) - end - end - end - end - - describe "#crypttab" do - context "when the filesystem contains a Windows system" do - let(:device_name) { "/dev/sda1" } - - let(:windows_content) { true } - - it "returns nil" do - expect(subject.crypttab).to be_nil - end - end - - context "when the filesystem does not contain a Windows system" do - let(:device_name) { "/dev/sda3" } - - let(:tested_method) { :crypttab } - - before do - allow(File).to receive(:exist?).and_return(exists_crypttab) - end - - let(:exists_crypttab) { true } - - include_examples "Mount and umount actions" - - context "when the crypttab file does not exist" do - let(:exists_crypttab) { false } - - it "returns nil" do - expect(subject.crypttab).to be_nil - end - end - - context "when the crypttab file exists" do - let(:exists_crypttab) { true } - - it "returns the crypttab" do - expect(subject.crypttab).to be_a(Y2Storage::Crypttab) - end - end - end - end - - describe "#rpi_boot?" do - context "when the filesystem contains a Windows system" do - let(:device_name) { "/dev/sda1" } - - let(:windows_content) { true } - - it "returns false" do - expect(subject.rpi_boot?).to eq(false) - end - end - - context "when the filesystem does not contain a Windows system" do - let(:device_name) { "/dev/sda3" } - - let(:tested_method) { :rpi_boot? } - let(:existing_files) { [] } - let(:result_if_mount_fails) { false } - - before do - allow(File).to receive(:exist?) do |name| - existing_files.include?(name) - end - end - - include_examples "Mount and umount actions" - - context "when there is no file called bootcode.bin or BOOTCODE.bin" do - let(:existing_files) { %w[/foo /bar] } - - it "returns false" do - expect(subject.rpi_boot?).to eq false - end - end - - context "when there is a file called bootcode.bin" do - let(:existing_files) { %w[/foo /bar /bootcode.bin] } - - it "returns true" do - expect(subject.rpi_boot?).to eq true - end - end - - context "when there is a file called BOOTCODE.BIN" do - let(:existing_files) { %w[/BOOTCODE.BIN] } - - it "returns true" do - expect(subject.rpi_boot?).to eq true - end - end - end - end - - describe "#windows?" do - context "when the architecture is not supported for Windows (non-PC system)" do - let(:architecture) { :s390 } - - it "returns false" do - expect(subject.windows?).to eq(false) - end - end - - context "when the architecture is supported for Windows (PC system)" do - let(:architecture) { :x86_64 } - - context "and the filesystem is created over a non-suitable Windows partition" do - let(:device_name) { "/dev/sda3" } - - it "returns false" do - expect(subject.windows?).to eq(false) - end - end - - context "when the filesystem is created over a suitable Windows partition" do - let(:device_name) { "/dev/sda1" } - - context "and the filesystem contains a Windows system" do - let(:windows_content) { true } - - it "returns true" do - expect(subject.windows?).to eq(true) - end - end - - context "and the filesystem does not contain a Windows system" do - let(:windows_content) { false } - - it "returns false" do - expect(subject.windows?).to eq(false) - end - end - - context "and the filesystem content cannot be inspected" do - before do - allow(filesystem).to receive(:detect_content_info).and_raise(Storage::Exception) - end - - it "returns false" do - expect(subject.windows?).to eq(false) - end - end - end - end - end -end diff --git a/test/y2storage/filesystem_reader_test.rb b/test/y2storage/filesystem_reader_test.rb new file mode 100755 index 0000000000..f8af3eddcb --- /dev/null +++ b/test/y2storage/filesystem_reader_test.rb @@ -0,0 +1,362 @@ +#!/usr/bin/env rspec +# Copyright (c) [2019] 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_relative "spec_helper" +require "y2storage" + +describe Y2Storage::FilesystemReader do + before do + fake_scenario(scenario) + + allow(Yast::Execute).to receive(:locally!) + + allow(Yast::OSRelease).to receive(:ReleaseName).and_return(release_name) + + allow(File).to receive(:exist?).and_return(true) + allow(File).to receive(:readlines).and_return([]) + + allow(filesystem).to receive(:windows_suitable?).and_return(windows_suitable) + allow(filesystem).to receive(:detect_content_info).and_return(content_info) + end + + let(:release_name) { "" } + + let(:content_info) { instance_double(Storage::ContentInfo, windows?: windows_content) } + + let(:windows_content) { false } + + let(:windows_suitable) { false } + + subject { described_class.new(filesystem) } + + let(:filesystem) { device.filesystem } + + let(:device) { fake_devicegraph.find_by_name(device_name) } + + let(:scenario) { "windows-linux-free-pc" } + + let(:device_name) { "/dev/sda3" } + + describe "#windows?" do + context "when the filesystem is not suitable for Windows" do + let(:windows_suitable) { false } + + it "returns false" do + expect(subject.windows?).to eq(false) + end + end + + context "when the filesystem is suitable for Windows" do + let(:windows_suitable) { true } + + context "and the filesystem contains a Windows system" do + let(:windows_content) { true } + + it "returns true" do + expect(subject.windows?).to eq(true) + end + end + + context "and the filesystem does not contain a Windows system" do + let(:windows_content) { false } + + it "returns false" do + expect(subject.windows?).to eq(false) + end + end + + context "and the filesystem content cannot be inspected" do + before do + allow(filesystem).to receive(:detect_content_info).and_raise(Storage::Exception) + end + + it "returns false" do + expect(subject.windows?).to eq(false) + end + end + end + end + + let(:cheetah_error) { Cheetah::ExecutionFailed.new([], "", nil, nil) } + + let(:mount_cmd) { ["/usr/bin/mount", "-o", "ro", "UUID=#{filesystem.uuid}", "/mnt"] } + + let(:umount_cmd) { ["/usr/bin/umount", "-R", "/mnt"] } + + RSpec.shared_examples "mounting" do |tested_method| + context "first time that is called" do + it "mounts the filesystem" do + expect(Yast::Execute).to receive(:locally!).with(*mount_cmd) + + subject.public_send(tested_method) + end + + it "umounts the filesystem" do + expect(Yast::Execute).to receive(:locally!).with(*umount_cmd) + + subject.public_send(tested_method) + end + + context "when mount fails" do + before do + allow(Yast::Execute).to receive(:locally!).with(*mount_cmd).and_raise(cheetah_error) + end + + it "does not raise an error" do + expect { subject.public_send(tested_method) }.to_not raise_error + end + + it "does not try to umount the filesystem" do + expect(Yast::Execute).to_not receive(:locally!).with(*umount_cmd) + + subject.public_send(tested_method) + end + end + + context "when umount fails" do + before do + allow(Yast::Execute).to receive(:locally!).with(*umount_cmd).and_raise(cheetah_error) + end + + it "does not raise an error" do + expect { subject.public_send(tested_method) }.to_not raise_error + end + end + end + + context "next time that is called" do + it "does not mount the filesystem again" do + expect(Yast::Execute).to receive(:locally!).with(*mount_cmd).once + + subject.public_send(tested_method) + subject.public_send(tested_method) + end + end + end + + describe "#release_name" do + context "when the filesystem contains a Windows system" do + let(:windows_suitable) { true } + + let(:windows_content) { true } + + it "returns nil" do + expect(subject.release_name).to be_nil + end + end + + context "when the filesystem does not contain a Windows system" do + let(:windows_suitable) { false } + + include_examples "mounting", :release_name + + context "and the filesystem is correctly mounted" do + before do + allow(Yast::Execute).to receive(:locally!).with(*mount_cmd) + + allow(File).to receive(:exist?).with(/os-release/).and_return(os_release_exist) + end + + context "and the os-release file exists" do + let(:os_release_exist) { true } + + let(:release_name) { "Open SUSE" } + + it "returns the release name" do + expect(subject.release_name).to eq(release_name) + end + end + + context "and the os-release file does not exist" do + let(:os_release_exist) { false } + + it "returns nil" do + expect(subject.release_name).to be_nil + end + end + end + + context "and the filesystem cannot be mounted" do + before do + allow(Yast::Execute).to receive(:locally!).with(*mount_cmd).and_raise(cheetah_error) + end + + it "returns nil" do + expect(subject.release_name).to be_nil + end + end + end + end + + describe "#rpi_boot?" do + context "when the filesystem contains a Windows system" do + let(:windows_suitable) { true } + + let(:windows_content) { true } + + it "returns false" do + expect(subject.rpi_boot?).to eq(false) + end + end + + context "when the filesystem does not contain a Windows system" do + let(:windows_suitable) { false } + + include_examples "mounting", :rpi_boot? + + context "and the filesystem is correctly mounted" do + before do + allow(Yast::Execute).to receive(:locally!).with(*mount_cmd) + + allow(File).to receive(:exist?) do |name| + existing_files.include?(name) + end + end + + context "but there is no file called bootcode.bin or BOOTCODE.bin" do + let(:existing_files) { ["/mnt/foo", "/mnt/bar"] } + + it "returns false" do + expect(subject.rpi_boot?).to eq(false) + end + end + + context "and there is a file called bootcode.bin" do + let(:existing_files) { ["/mnt/foo", "/mnt/bar", "/mnt/bootcode.bin"] } + + it "returns true" do + expect(subject.rpi_boot?).to eq(true) + end + end + + context "and there is a file called BOOTCODE.BIN" do + let(:existing_files) { ["/mnt/BOOTCODE.BIN"] } + + it "returns true" do + expect(subject.rpi_boot?).to eq(true) + end + end + end + + context "and the filesystem cannot be mounted" do + before do + allow(Yast::Execute).to receive(:locally!).with(*mount_cmd).and_raise(cheetah_error) + end + + it "returns false" do + expect(subject.rpi_boot?).to eq(false) + end + end + end + end + + describe "#fstab" do + context "when the filesystem contains a Windows system" do + let(:windows_suitable) { true } + + let(:windows_content) { true } + + it "returns nil" do + expect(subject.fstab).to be_nil + end + end + + context "when the filesystem does not contain a Windows system" do + let(:windows_suitable) { false } + + include_examples "mounting", :fstab + + context "and the filesystem is correctly mounted" do + before do + allow(Yast::Execute).to receive(:locally!).with(*mount_cmd) + + allow(File).to receive(:exist?).with("/mnt/etc/fstab").and_return(exists_fstab) + end + + context "and the fstab file does not exist" do + let(:exists_fstab) { false } + + it "returns nil" do + expect(subject.fstab).to be_nil + end + end + + context "and the fstab file exists" do + let(:exists_fstab) { true } + + before do + allow(File).to receive(:readlines).with("/mnt/etc/fstab").and_return(["fstab ", "content"]) + end + + it "returns the fstab content" do + expect(subject.fstab).to eq("fstab content") + end + end + end + end + end + + describe "#crypttab" do + context "when the filesystem contains a Windows system" do + let(:windows_suitable) { true } + + let(:windows_content) { true } + + it "returns nil" do + expect(subject.crypttab).to be_nil + end + end + + context "when the filesystem does not contain a Windows system" do + let(:windows_suitable) { false } + + include_examples "mounting", :crypttab + + context "and the filesystem is correctly mounted" do + before do + allow(Yast::Execute).to receive(:locally!).with(*mount_cmd) + + allow(File).to receive(:exist?).with("/mnt/etc/crypttab").and_return(exists_crypttab) + end + + context "and the crypttab file does not exist" do + let(:exists_crypttab) { false } + + it "returns nil" do + expect(subject.crypttab).to be_nil + end + end + + context "and the crypttab file exists" do + let(:exists_crypttab) { true } + + before do + allow(File) + .to receive(:readlines).with("/mnt/etc/crypttab").and_return(["crypttab ", "content"]) + end + + it "returns the crypttab content" do + expect(subject.crypttab).to eq("crypttab content") + end + end + end + end + end +end From c28b1631573b4ee0a7cd13efbfb02c82fd476936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Wed, 4 Dec 2019 12:11:51 +0000 Subject: [PATCH 2/6] Add methods to Filesystem - Memorize info that requires to mount the filesystem --- src/lib/y2storage/blk_device.rb | 9 + .../boot_requirements_strategies/raspi.rb | 5 +- src/lib/y2storage/filesystems/base.rb | 154 +++++++- src/lib/y2storage/filesystems/type.rb | 27 +- src/lib/y2storage/partition.rb | 16 +- test/y2storage/blk_device_test.rb | 9 + .../boot_requirements_checker_raspi_test.rb | 10 +- test/y2storage/filesystems/base_test.rb | 352 +++++++++++++++++- test/y2storage/filesystems/type_test.rb | 28 +- test/y2storage/partition_test.rb | 46 ++- 10 files changed, 612 insertions(+), 44 deletions(-) diff --git a/src/lib/y2storage/blk_device.rb b/src/lib/y2storage/blk_device.rb index afc0dc450f..29d07adb2c 100644 --- a/src/lib/y2storage/blk_device.rb +++ b/src/lib/y2storage/blk_device.rb @@ -631,6 +631,15 @@ def in_network? false end + # Whether the block device fulfills conditions to be used for a Windows system + # + # Note that only partitions can be used for installing Windows. + # + # @return [Boolean] + def windows_suitable? + false + end + protected # Values for volume specification matching diff --git a/src/lib/y2storage/boot_requirements_strategies/raspi.rb b/src/lib/y2storage/boot_requirements_strategies/raspi.rb index 092a3cf537..b85698125a 100644 --- a/src/lib/y2storage/boot_requirements_strategies/raspi.rb +++ b/src/lib/y2storage/boot_requirements_strategies/raspi.rb @@ -1,4 +1,4 @@ -# Copyright (c) [2018] SUSE LLC +# Copyright (c) [2018-2019] SUSE LLC # # All Rights Reserved. # @@ -18,7 +18,6 @@ # find current contact information at www.suse.com. require "y2storage/boot_requirements_strategies/uefi" -require "y2storage/existing_filesystem" module Y2Storage module BootRequirementsStrategies @@ -114,7 +113,7 @@ def rpi_boot?(partition) filesystem = partition.direct_blk_filesystem return false if filesystem.nil? || !filesystem.type.is?(:vfat) - ExistingFilesystem.new(filesystem).rpi_boot? + filesystem.rpi_boot? end # First partition in a disk diff --git a/src/lib/y2storage/filesystems/base.rb b/src/lib/y2storage/filesystems/base.rb index f70da924f3..26cf942a34 100644 --- a/src/lib/y2storage/filesystems/base.rb +++ b/src/lib/y2storage/filesystems/base.rb @@ -1,4 +1,4 @@ -# Copyright (c) [2017] SUSE LLC +# Copyright (c) [2017-2019] SUSE LLC # # All Rights Reserved. # @@ -17,9 +17,14 @@ # To contact SUSE LLC about this file by physical or electronic mail, you may # find current contact information at www.suse.com. +require "tempfile" + require "y2storage/storage_class_wrapper" require "y2storage/mountable" require "y2storage/filesystems/type" +require "y2storage/filesystem_reader" +require "y2storage/fstab" +require "y2storage/crypttab" module Y2Storage module Filesystems @@ -130,6 +135,93 @@ def match_fstab_spec?(spec) false end + # Whether the filesystem is suitable for a root filesystem + # + # @see Filesystems::Type#root_ok? + # + # @return [Boolean] + def root_suitable? + type.root_ok? + end + + # Whether the filesystem is suitable for a Windows system + # + # @see Filesystems::Type#windows_ok? + # @see BlkDevice#windows_suitable? + # + # @return [Boolean] + def windows_suitable? + type.windows_ok? && blk_devices.first.windows_suitable? + end + + # Whether the filesystem contains a Windows system + # + # Note that the filesystem might be mounted when requested for first time. + # + # @return [Boolean] + def windows_system? + return false unless windows_suitable? + + !!fs_attribute(:windows) + end + + # Whether the filesystem contains a Linux system + # + # Note that the filesystem might be mounted when requested for first time. + # + # @return [Boolean] + def linux_system? + !release_name.nil? + end + + # Name of the system allocated by the filesystem + # + # For a Windows system it simply returns "Windows" as system name. For Linux it tries to read the + # release name. + # + # Note that the filesystem might be mounted when requested for first time. + # + # @return [String, nil] nil if the system cannot be detected + def system_name + windows_system? ? "Windows" : release_name + end + + # Release name of Linux system allocated by the filesystem + # + # Note that the filesystem might be mounted when requested for first time. + # + # @return [String, nil] nil if the system cannot be detected + def release_name + fs_attribute(:release_name) + end + + # Whether the filesystem contains the Raspberry Pi boot code in the root path + # + # Note that the filesystem might be mounted when requested for first time. + # + # @return [Boolean] + def rpi_boot? + !!fs_attribute(:rpi_boot) + end + + # Retrieves the fstab from the filesystem + # + # Note that the filesystem might be mounted when requested for first time. + # + # @return [Y2Storage::Fstab, nil] nil if the fstab file is not found + def fstab + @fstab ||= etc_file("fstab") + end + + # Retrieves the crypttab from the filesystem + # + # Note that the filesystem might be mounted when requested for first time. + # + # @return [Y2Storage::Crypttab, nil] nil if the crypttab file is not found + def crypttab + @crypttab ||= etc_file("crypttab") + end + protected # @see Device#is? @@ -139,6 +231,7 @@ def types_for_is # Value to return as fallback when the free space cannot be computed FREE_SPACE_FALLBACK = DiskSize.zero + def compute_free_space # e.g. nfs where blk_devices cannot be queried return FREE_SPACE_FALLBACK unless respond_to?(:blk_devices) @@ -151,6 +244,65 @@ def compute_free_space # but there is high chance we can't use it with libstorage, so better act like zero device. FREE_SPACE_FALLBACK end + + # Filesystem attribute obtained after mounting the filesystem + # + # Note that the filesystem is only mounted the first time that an attribute is requested. + # + # @param attr [Symbol] :windows, :release_name, :rpi_boot, :fstab, :crypttab + # @return [Object] attribute value + def fs_attribute(attr) + read_fs_attributes unless userdata_value(:fs_attributes_already_read) + + userdata_value(attr) + end + + # Reads and saves attributes from a probed filesystem + # + # It requires to mount the filesystem. + # + # @see Y2Storage::FilesystemReader + def read_fs_attributes + save_userdata(:fs_attributes_already_read, true) + + return unless exists_in_probed? + + reader = FilesystemReader.new(self) + + save_userdata(:windows, reader.windows?) + save_userdata(:release_name, reader.release_name) + save_userdata(:rpi_boot, reader.rpi_boot?) + save_userdata(:fstab, reader.fstab) + save_userdata(:crypttab, reader.crypttab) + end + + # Generates an etc file object + # + # Note that a temporary file is created with the content of the etc file because libstorage-ng API + # requires a file path, see {Y2Storage::Fstab} and {Y2Storage::Crypttab}. + # + # @param file_name [String] "fstab" or "crypttab" + # @return [Y2Storage::Fstab, Y2Storage::Crypttab, nil] nil if the file is not found + def etc_file(file_name) + file_content = fs_attribute(file_name.to_sym) + + return nil if file_content.nil? + + file_object = nil + + Tempfile.open("yast-storage-ng") do |file| + file.write(file_content) + file.rewind + + if file_name == "fstab" + file_object = Fstab.new(file.path, self) + elsif file_name == "crypttab" + file_object = Crypttab.new(file.path, self) + end + end + + file_object + end end end end diff --git a/src/lib/y2storage/filesystems/type.rb b/src/lib/y2storage/filesystems/type.rb index f5be2a7085..73cf4e1d4a 100644 --- a/src/lib/y2storage/filesystems/type.rb +++ b/src/lib/y2storage/filesystems/type.rb @@ -1,4 +1,4 @@ -# Copyright (c) [2017] SUSE LLC +# Copyright (c) [2017-2019] SUSE LLC # # All Rights Reserved. # @@ -167,6 +167,8 @@ class Type # filesystems that can embed grub GRUB_FILESYSTEMS = [:ext2, :ext3, :ext4, :btrfs] + WINDOWS_FILESYSTEMS = [:ntfs, :vfat] + private_constant :PROPERTIES, :ROOT_FILESYSTEMS, :HOME_FILESYSTEMS, :COMMON_FSTAB_OPTIONS, :EXT_FSTAB_OPTIONS, :LEGACY_ROOT_FILESYSTEMS, :LEGACY_HOME_FILESYSTEMS, :ZIPL_FILESYSTEMS, :JOURNAL_OPTIONS, @@ -221,22 +223,28 @@ def self.grub_filesystems GRUB_FILESYSTEMS.map { |f| find(f) } end + # Allowed filesystems for Windows + # + # @return [Array] + def self.windows_filesystems + WINDOWS_FILESYSTEMS.map { |f| find(f) } + end + # Check if filesystem is usable as root (mountpoint "/") filesystem. # - # return [Boolean] + # @return [Boolean] # # @example # devicegraph.filesystems.each do |fs| # puts "#{fs.type}: #{fs.type.root_ok?}" # end - # def root_ok? Type.root_filesystems.include?(self) end # Check if filesystem was usable as root (mountpoint "/") filesystem. # - # return [Boolean] + # @return [Boolean] # # @example # devicegraph.filesystems.each do |fs| @@ -249,7 +257,7 @@ def legacy_root? # Check if filesystem is usable as home (mountpoint "/home") filesystem. # - # return [Boolean] + # @return [Boolean] # # @example # devicegraph.filesystems.each do |fs| @@ -277,7 +285,7 @@ def grub_ok? # Check if filesystem was usable as home (mountpoint "/home") filesystem. # - # return [Boolean] + # @return [Boolean] # # @example # devicegraph.filesystems.each do |fs| @@ -288,6 +296,13 @@ def legacy_home? Type.legacy_home_filesystems.include?(self) end + # Whether is usable for installing a Windows system. + # + # @return [Boolean] + def windows_ok? + Type.windows_filesystems.include?(self) + end + # Human readable text for a filesystem # # @return [String] diff --git a/src/lib/y2storage/partition.rb b/src/lib/y2storage/partition.rb index c54014d544..ba53136c2c 100644 --- a/src/lib/y2storage/partition.rb +++ b/src/lib/y2storage/partition.rb @@ -1,4 +1,4 @@ -# Copyright (c) [2017] SUSE LLC +# Copyright (c) [2017-2019] SUSE LLC # # All Rights Reserved. # @@ -267,13 +267,6 @@ def swap? id.is?(:swap) && formatted_as?(:swap) end - # Whether the partition fulfills conditions to be used for a Windows system - # - # @return [Boolean] - def suitable_for_windows? - type.is?(:primary) && id.is?(:windows_system) - end - # Whether it is a (valid) EFI System partition # # @return [Boolean] @@ -286,6 +279,13 @@ def in_network? partitionable.in_network? end + # Whether the partition fulfills conditions to be used for a Windows system + # + # @return [Boolean] + def windows_suitable? + !disk.nil? && type.is?(:primary) && id.is?(:windows_system) + end + protected # Values for volume specification matching diff --git a/test/y2storage/blk_device_test.rb b/test/y2storage/blk_device_test.rb index e122e53924..8e7534b21d 100755 --- a/test/y2storage/blk_device_test.rb +++ b/test/y2storage/blk_device_test.rb @@ -1,4 +1,5 @@ #!/usr/bin/env rspec + # Copyright (c) [2017-2019] SUSE LLC # # All Rights Reserved. @@ -1282,4 +1283,12 @@ def create_encrypted_partition(disk, slot_index) end end end + + describe "#windows_suitable?" do + let(:device_name) { "/dev/sdb" } + + it "returns false" do + expect(subject.windows_suitable?).to eq(false) + end + end end diff --git a/test/y2storage/boot_requirements_checker_raspi_test.rb b/test/y2storage/boot_requirements_checker_raspi_test.rb index 6f5f15e0e2..76f31991d5 100755 --- a/test/y2storage/boot_requirements_checker_raspi_test.rb +++ b/test/y2storage/boot_requirements_checker_raspi_test.rb @@ -1,5 +1,5 @@ #!/usr/bin/env rspec -# Copyright (c) [2018] SUSE LLC +# Copyright (c) [2018-2019] SUSE LLC # # All Rights Reserved. # @@ -47,8 +47,10 @@ def rpi_boot_double(name) let(:rpi_boot_sda) { rpi_boot_double("/dev/sda1") } let(:rpi_boot_sdb) { rpi_boot_double("/dev/sdb1") } - let(:rpi_boot_fs) { double("BlkFilesystem", type: Y2Storage::Filesystems::Type::VFAT) } - let(:rpi_boot_existing_fs) { double("ExistingFilesystem", rpi_boot?: true) } + let(:rpi_boot_fs) do + instance_double(Y2Storage::Filesystems::BlkFilesystem, + type: Y2Storage::Filesystems::Type::VFAT, rpi_boot?: true) + end before do allow(dev_sda).to receive(:efi_partitions).and_return efi_partitions @@ -58,8 +60,6 @@ def rpi_boot_double(name) allow(dev_sdb).to receive(:efi_partitions).and_return [] allow(dev_sdb).to receive(:partitions).and_return sdb_partitions allow(dev_sdb).to receive(:partition_table).and_return sdb_ptable - - allow(Y2Storage::ExistingFilesystem).to receive(:new).and_return rpi_boot_existing_fs end RSpec.shared_context "Raspberry Pi partitions" do diff --git a/test/y2storage/filesystems/base_test.rb b/test/y2storage/filesystems/base_test.rb index cf674953cb..06087f957d 100755 --- a/test/y2storage/filesystems/base_test.rb +++ b/test/y2storage/filesystems/base_test.rb @@ -1,5 +1,5 @@ #!/usr/bin/env rspec -# Copyright (c) [2017] SUSE LLC +# Copyright (c) [2017-2019] SUSE LLC # # All Rights Reserved. # @@ -22,12 +22,14 @@ require "y2storage" describe Y2Storage::Filesystems::Base do - before do fake_scenario(scenario) end + let(:scenario) { "mixed_disks_btrfs" } - let(:blk_device) { Y2Storage::BlkDevice.find_by_name(fake_devicegraph, "/dev/sda2") } + + let(:blk_device) { fake_devicegraph.find_by_name("/dev/sda2") } + subject(:filesystem) { blk_device.blk_filesystem } describe "#free_space" do @@ -75,4 +77,348 @@ end end end + + describe "#root_suitable?" do + before do + allow(subject).to receive(:type).and_return(type) + end + + let(:type) { instance_double(Y2Storage::Filesystems::Type, root_ok?: suitable) } + + context "when the type is suitable for root" do + let(:suitable) { true } + + it "returns true" do + expect(subject.root_suitable?).to eq(true) + end + end + + context "when the type is not suitable for root" do + let(:suitable) { false } + + it "returns false" do + expect(subject.root_suitable?).to eq(false) + end + end + end + + describe "#windows_suitable?" do + before do + allow(subject).to receive(:type).and_return(type) + end + + let(:type) { instance_double(Y2Storage::Filesystems::Type, windows_ok?: type_suitable) } + + context "when the type is not suitable for Windows" do + let(:type_suitable) { false } + + it "returns false" do + expect(subject.windows_suitable?).to eq(false) + end + + context "when the type is suitable for Windows" do + let(:type_suitable) { true } + + before do + allow(subject).to receive(:blk_devices).and_return([blk_device]) + + allow(blk_device).to receive(:windows_suitable?).and_return(device_suitable) + end + + context "but its device is not suitable for Windows" do + let(:device_suitable) { false } + + it "returns false" do + expect(subject.windows_suitable?).to eq(false) + end + end + + context "and its device is suitable for Windows" do + let(:device_suitable) { true } + + it "returns true" do + expect(subject.windows_suitable?).to eq(true) + end + end + end + end + end + + shared_examples "creating_reader" do |tested_method| + it "creates a filesystem reader only the first time is called" do + expect(Y2Storage::FilesystemReader).to receive(:new).once.and_return(reader) + + subject.public_send(tested_method) + subject.public_send(tested_method) + subject.public_send(tested_method) + end + end + + describe "#windows_system?" do + before do + allow(subject).to receive(:windows_suitable?).and_return(suitable) + end + + context "when the filesystem is not suitable for Windows" do + let(:suitable) { false } + + it "returns false" do + expect(subject.windows_system?).to eq(false) + end + end + + context "when the filesystem is suitable for Windows" do + let(:suitable) { true } + + before do + allow(Y2Storage::FilesystemReader).to receive(:new).with(subject).and_return(reader) + end + + let(:reader) { double(Y2Storage::FilesystemReader, windows?: windows).as_null_object } + + context "but it does not contain a Windows" do + let(:windows) { false } + + include_examples "creating_reader", :windows_system? + + it "returns false" do + expect(subject.windows_system?).to eq(false) + end + end + + context "and it contains a Windows" do + let(:windows) { true } + + include_examples "creating_reader", :windows_system? + + it "returns true" do + expect(subject.windows_system?).to eq(true) + end + end + end + end + + context "#linux_system?" do + before do + allow(Y2Storage::FilesystemReader).to receive(:new).with(subject).and_return(reader) + end + + let(:reader) { double(Y2Storage::FilesystemReader, release_name: release_name).as_null_object } + + context "when the filesystem contains a release name" do + let(:release_name) { "Linux" } + + include_examples "creating_reader", :linux_system? + + it "returns true" do + expect(subject.linux_system?).to eq(true) + end + end + + context "when the filesystem does not contain a release name" do + let(:release_name) { nil } + + include_examples "creating_reader", :linux_system? + + it "returns false" do + expect(subject.linux_system?).to eq(false) + end + end + end + + context "#system_name" do + before do + allow(subject).to receive(:windows_suitable?).and_return(true) + + allow(Y2Storage::FilesystemReader).to receive(:new).with(subject).and_return(reader) + end + + let(:reader) do + double(Y2Storage::FilesystemReader, + windows?: windows, + release_name: release_name).as_null_object + end + + context "when the filesystem contains a Windows" do + let(:windows) { true } + + let(:release_name) { nil } + + include_examples "creating_reader", :system_name + + it "returns 'Windows'" do + expect(subject.system_name).to eq("Windows") + end + end + + context "when the filesystem contains a Linux" do + let(:windows) { false } + + let(:release_name) { "Linux" } + + include_examples "creating_reader", :system_name + + it "returns the Linux release name" do + expect(subject.system_name).to eq("Linux") + end + end + end + + context "#release_name" do + before do + allow(Y2Storage::FilesystemReader).to receive(:new).with(subject).and_return(reader) + end + + let(:reader) { double(Y2Storage::FilesystemReader, release_name: release_name).as_null_object } + + context "when the filesystem contains a release name" do + let(:release_name) { "Linux" } + + include_examples "creating_reader", :release_name + + it "returns the release name" do + expect(subject.release_name).to eq("Linux") + end + end + + context "when the filesystem does not contain a release name" do + let(:release_name) { nil } + + include_examples "creating_reader", :release_name + + it "returns nil" do + expect(subject.release_name).to be_nil + end + end + end + + context "#rpi_boot?" do + before do + allow(Y2Storage::FilesystemReader).to receive(:new).with(subject).and_return(reader) + end + + let(:reader) { double(Y2Storage::FilesystemReader, rpi_boot?: rpi_boot).as_null_object } + + context "when the filesystem contains files for rpi bootloader" do + let(:rpi_boot) { true } + + include_examples "creating_reader", :rpi_boot? + + it "returns true" do + expect(subject.rpi_boot?).to eq(true) + end + end + + context "when the filesystem does not contain files for rpi bootloader" do + let(:rpi_boot) { false } + + include_examples "creating_reader", :rpi_boot? + + it "returns false" do + expect(subject.rpi_boot?).to eq(false) + end + end + end + + shared_examples "creating_temporary_file" do |tested_method| + it "creates a temporary files only the first time is called" do + allow(Tempfile).to receive(:open).with("yast-storage-ng").once.and_yield(file) + + subject.public_send(tested_method) + subject.public_send(tested_method) + subject.public_send(tested_method) + end + end + + context "#fstab" do + before do + allow(Y2Storage::FilesystemReader).to receive(:new).with(subject).and_return(reader) + end + + let(:reader) { double(Y2Storage::FilesystemReader, fstab: fstab_content).as_null_object } + + context "when the filesystem contains a fstab" do + let(:fstab_content) { "fstab content" } + + before do + allow(Tempfile).to receive(:open).with("yast-storage-ng").and_yield(file) + + allow(Y2Storage::Fstab).to receive(:new).with(file.path, subject).and_return(fstab) + end + + let(:file) { double("Tempfile", path: "/tmp/something").as_null_object } + + let(:fstab) { instance_double(Y2Storage::Fstab) } + + include_examples "creating_reader", :fstab + + it "saves the fstab content into a temporary file" do + expect(file).to receive(:write).with(fstab_content) + + subject.fstab + end + + include_examples "creating_temporary_file", :fstab + + it "returns a Fstab object created from the temporary file" do + expect(subject.fstab).to eq(fstab) + end + end + + context "when the filesystem does not contain a fstab" do + let(:fstab_content) { nil } + + include_examples "creating_reader", :fstab + + it "returns nil" do + expect(subject.fstab).to be_nil + end + end + end + + context "#crypttab" do + before do + allow(Y2Storage::FilesystemReader).to receive(:new).with(subject).and_return(reader) + end + + let(:reader) { double(Y2Storage::FilesystemReader, crypttab: crypttab_content).as_null_object } + + context "when the filesystem contains a crypttab" do + let(:crypttab_content) { "crypttab content" } + + before do + allow(Tempfile).to receive(:open).with("yast-storage-ng").and_yield(file) + + allow(Y2Storage::Crypttab).to receive(:new).with(file.path, subject).and_return(crypttab) + end + + let(:file) { double("Tempfile", path: "/tmp/something").as_null_object } + + let(:crypttab) { instance_double(Y2Storage::Crypttab) } + + include_examples "creating_reader", :crypttab + + it "saves the crypttab content into a temporary file" do + expect(file).to receive(:write).with(crypttab_content) + + subject.crypttab + end + + include_examples "creating_temporary_file", :crypttab + + it "returns a Crypttab object created from the temporary file" do + expect(subject.crypttab).to eq(crypttab) + end + end + + context "when the filesystem does not contain a crypttab" do + let(:crypttab_content) { nil } + + include_examples "creating_reader", :crypttab + + it "returns nil" do + expect(subject.crypttab).to be_nil + end + end + end end diff --git a/test/y2storage/filesystems/type_test.rb b/test/y2storage/filesystems/type_test.rb index b120800773..33852b9d16 100755 --- a/test/y2storage/filesystems/type_test.rb +++ b/test/y2storage/filesystems/type_test.rb @@ -1,5 +1,5 @@ #!/usr/bin/env rspec -# Copyright (c) [2017] SUSE LLC +# Copyright (c) [2017-2019] SUSE LLC # # All Rights Reserved. # @@ -254,4 +254,30 @@ expect(chars).not_to include("\n") end end + + describe ".windows_filesystems" do + it "returns a list" do + expect(described_class.windows_filesystems).to be_a(Array) + end + + it "only includes ntfs and vfat" do + expect(described_class.windows_filesystems.map(&:to_sym)).to contain_exactly(:ntfs, :vfat) + end + end + + describe "#windows_ok?" do + it "returns true for ntfs" do + expect(described_class::NTFS.windows_ok?).to eq(true) + end + + it "returns true for vfat" do + expect(described_class::VFAT.windows_ok?).to eq(true) + end + + it "returns false otherwise" do + types = described_class.all.reject { |t| t.is?(:ntfs, :vfat) } + + expect(types.map(&:windows_ok?)).to all(be(false)) + end + end end diff --git a/test/y2storage/partition_test.rb b/test/y2storage/partition_test.rb index 8c326a049b..2aae0a61ea 100755 --- a/test/y2storage/partition_test.rb +++ b/test/y2storage/partition_test.rb @@ -1,5 +1,5 @@ #!/usr/bin/env rspec -# Copyright (c) [2017] SUSE LLC +# Copyright (c) [2017-2019] SUSE LLC # # All Rights Reserved. # @@ -544,9 +544,7 @@ end end - describe "#suitable_for_windows?" do - let(:scenario) { "mixed_disks" } - + describe "#windows_suitable?" do subject(:partition) { fake_devicegraph.find_by_name(device_name) } before do @@ -555,30 +553,44 @@ let(:id) { Y2Storage::PartitionId::LINUX } - context "when it is not a primary partition" do - let(:device_name) { "/dev/sdb5" } + context "when the partition does not belongs to a disk" do + let(:scenario) { "partitioned_md" } + + let(:device_name) { "/dev/md0p1" } it "returns false" do - expect(subject.suitable_for_windows?).to eq(false) + expect(subject.windows_suitable?).to eq(false) end end - context "when it is a primary partition" do - let(:device_name) { "/dev/sda1" } + context "when the partition belongs to a disk" do + let(:scenario) { "mixed_disks" } - context "and it has 'windows_system' id" do - let(:id) { Y2Storage::PartitionId::NTFS } + context "but it is not a primary partition" do + let(:device_name) { "/dev/sdb5" } - it "returns true" do - expect(subject.suitable_for_windows?).to eq(true) + it "returns false" do + expect(subject.windows_suitable?).to eq(false) end end - context "and it has no 'windows_system' id" do - let(:id) { Y2Storage::PartitionId::LVM } + context "and it is a primary partition" do + let(:device_name) { "/dev/sda1" } - it "returns false" do - expect(subject.suitable_for_windows?).to eq(false) + context "and it has 'windows_system' id" do + let(:id) { Y2Storage::PartitionId::NTFS } + + it "returns true" do + expect(subject.windows_suitable?).to eq(true) + end + end + + context "and it has no 'windows_system' id" do + let(:id) { Y2Storage::PartitionId::LVM } + + it "returns false" do + expect(subject.windows_suitable?).to eq(false) + end end end end From b693809837f7bdd621dcfe875e40b441a5e256a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Wed, 4 Dec 2019 12:13:38 +0000 Subject: [PATCH 3/6] Adapt DiskAnalyzer - Add method to check if there is a Windows system --- src/lib/y2storage/disk_analyzer.rb | 237 ++++++++++----------------- src/lib/y2storage/partitionable.rb | 13 +- test/y2storage/disk_analyzer_test.rb | 136 +++++++++++++-- 3 files changed, 209 insertions(+), 177 deletions(-) diff --git a/src/lib/y2storage/disk_analyzer.rb b/src/lib/y2storage/disk_analyzer.rb index d4e4ccb573..da93e385ea 100755 --- a/src/lib/y2storage/disk_analyzer.rb +++ b/src/lib/y2storage/disk_analyzer.rb @@ -1,8 +1,4 @@ -#!/usr/bin/env ruby -# -# encoding: utf-8 - -# Copyright (c) [2015] SUSE LLC +# Copyright (c) [2015-2019] SUSE LLC # # All Rights Reserved. # @@ -23,13 +19,11 @@ require "yast" require "storage" -require "fileutils" require "y2packager/repository" require "y2storage/disk_size" require "y2storage/blk_device" require "y2storage/lvm_pv" require "y2storage/partition_id" -require "y2storage/existing_filesystem" Yast.import "Arch" @@ -47,20 +41,22 @@ module Y2Storage class DiskAnalyzer include Yast::Logger - NO_INSTALLATION_IDS = - [ - PartitionId::SWAP, - PartitionId::EXTENDED, - PartitionId::LVM - ] - - # Maximum number of checks for "expensive" operations. - DEFAULT_CHECK_LIMIT = 10 - + # Constructor + # + # @param devicegraph [Devicegraph] def initialize(devicegraph) @devicegraph = devicegraph end + # Whether there is a Windows system + # + # @param disks [Disk, String] disks to analyze. All disks by default. + def windows_system?(*disks) + return false unless windows_architecture? + + filesystems_collection(*disks).any?(&:windows_system?) + end + # Partitions containing an installation of MS Windows # # This involves mounting any Windows-like partition to check if there are @@ -69,83 +65,80 @@ def initialize(devicegraph) # @param disks [Disk, String] disks to analyze. All disks by default. # @return [Array] def windows_partitions(*disks) - data_for(*disks, :windows_partitions) { |d| find_windows_partitions(d) } + return [] unless windows_architecture? + + windows_filesystems(*disks).flat_map(&:blk_devices) end - # Linux partitions. + # Partitions with a proper Linux partition Id # # @see PartitionId.linux_system_ids # # @param disks [Disk, String] disks to analyze. All disks by default. # @return [Array] def linux_partitions(*disks) - data_for(*disks, :linux_partitions, &:linux_system_partitions) + disks_collection(*disks).flat_map(&:linux_system_partitions) end - # Release names of installed systems for every disk. + # Name of installed systems # # @param disks [Disk, String] disks to analyze. All disks by default. # @return [Array] release names def installed_systems(*disks) - data_for(*disks, :installed_systems) { |d| find_installed_systems(d) } + windows_systems(*disks) + linux_systems(*disks) end - # Release names of installed Windows systems for every disk. + # Name of installed Windows systems # # @param disks [Disk, String] disks to analyze. All disks by default. # @return [Array] release names def windows_systems(*disks) - data_for(*disks, :windows_systems) { |d| find_windows_systems(d) } + windows_filesystems(*disks).map(&:system_name).compact end - # Release names of installed Linux systems for every disk. + # Release name of installed Linux systems # # @param disks [Disk, String] disks to analyze. All disks by default. # @return [Array] release names def linux_systems(*disks) - data_for(*disks, :linux_systems) { |d| find_linux_systems(d) } + linux_suitable_filesystems(*disks).map(&:release_name).compact end # All fstabs found in the system # - # FIXME: this method is not using the {#data_for} caching mechanism. The reason - # is because fstab information needs to be stored by filesystem, but {#data_for} - # saves information by disk. As a consequence, this method could mount a device - # that was already mounted previously to read some information on it, for - # example the {#installed_systems}. + # Note that all filesystems are considered here, including filesystems over LVM LVs, see + # {#all_linux_suitable_filesystems}. # # @return [Array] def fstabs - return @fstabs if @fstabs - - save_config_files - @fstabs + @fstabs ||= all_linux_suitable_filesystems.map(&:fstab).compact end # All crypttabs found in the system # - # FIXME: this method is not using the {#data_for} caching mechanism. The reason - # is because crypttab information needs to be stored by filesystem, but {#data_for} - # saves information by disk. As a consequence, this method could mount a device - # that was already mounted previously to read some information on it, for - # example the {#installed_systems}. + # Note that all filesystems are considered here, including filesystems over LVM LVs, see + # {#all_linux_suitable_filesystems}. # # @return [Array] def crypttabs - return @crypttabs if @crypttabs - - save_config_files - @crypttabs + @crypttabs ||= all_linux_suitable_filesystems.map(&:crypttab).compact end # Disks that are suitable for installing Linux. # - # @return [Array] candidate disks + # Finds devices (disk devices and software RAIDs) that are suitable for installing Linux + # + # From fate#326573 on, software RAIDs with partition table or without children are also + # considered as valid candidates. + # + # @return [Array] candidate def candidate_disks return @candidate_disks if @candidate_disks - @candidate_disks = find_candidate_disks + @candidate_disks = candidate_software_raids + candidate_disk_devices + log.info("Found candidate disks: #{@candidate_disks}") + @candidate_disks end @@ -159,20 +152,15 @@ def device_by_name(name) private + # @return [Devicegraph] attr_reader :devicegraph - # Gets data for a set of disks, stores it and returns that data. + # Whether the architecture of the system is supported by MS Windows # - # @param disks [Disk, String] disks to analyze. All disks by default. - # @param data [Symbol] data name. - def data_for(*disks, data) - @disks_data ||= {} - @disks_data[data] ||= {} - disks = disks_collection(disks) - disks.each do |disk| - @disks_data[data][disk.name] ||= yield(disk) - end - disks.map { |d| @disks_data[data][d.name] }.flatten.compact + # @return [Boolean] + def windows_architecture? + # Should we include ARM here? + Yast::Arch.x86_64 || Yast::Arch.i386 end # Obtains a list of disk devices, software RAIDs, and bcaches @@ -181,7 +169,7 @@ def data_for(*disks, data) # # @param disks [Array] blk device to analyze. # @return [Array] a list of blk devices - def disks_collection(disks) + def disks_collection(*disks) return default_disks_collection if disks.empty? disks.map! { |d| d.is_a?(String) ? BlkDevice.find_by_name(devicegraph, d) : d } @@ -198,99 +186,58 @@ def default_disks_collection devicegraph.disk_devices + devicegraph.software_raids + devicegraph.bcaches end - # @see #windows_partitions - def find_windows_partitions(disk) - disk.partitions.select { |p| windows_partition?(p) } - end - - # Check if the partition contains a MS Windows system that could possibly be resized. + # All partitions from the given disks # - # @param partition [Partition] partition to check - # @return [Boolean] - def windows_partition?(partition) - return false unless partition.formatted? - - filesystem = partition.filesystem - ExistingFilesystem.new(filesystem).windows? - end - - # Obtain release names of installed systems in a disk. + # @see #disks_collection # - # @param disk [Disk] disk to check - # @return [Array] release names - def find_installed_systems(disk) - windows_systems(disk) + linux_systems(disk) + # @param disks [Array] + # @return [Array] + def partitions_collection(*disks) + disks_collection(*disks).flat_map(&:partitions) end - # Obtain release names of installed Windows systems in a disk. + # All filesystems from the given disks # - # @param disk [Disk] disk to check - # @return [Array] release names - def find_windows_systems(disk) - systems = [] - systems << "Windows" unless windows_partitions(disk).empty? - systems - end - - # Obtain release names of installed Linux systems in a disk. + # @see #disks_collection + # @see #partitions_collection # - # @param disk [Disk] disk to check - # @return [Array] release names - def find_linux_systems(disk) - filesystems = linux_partitions(disk).map(&:filesystem) - filesystems << disk.filesystem - filesystems.compact! - filesystems.map { |f| release_name(f) }.compact - end + # @param disks [Array] + # @return [Array] + def filesystems_collection(*disks) + blk_devices = disks_collection(*disks) + partitions_collection(*disks) - # Saves all found fstab and crypttab files - def save_config_files - @fstabs, @crypttabs = find_config_files + blk_devices.map(&:filesystem).compact end - # Finds fstab and crypttab files in all suitable filesystems for root + # All filesystems that contain a Windows system from the given disks # - # @see #suitable_root_filesystems + # @see #filesystems_collection # - # @return [Array, Array>] - def find_config_files - fstabs = [] - crypttabs = [] - - suitable_root_filesystems.each do |filesystem| - fs = ExistingFilesystem.new(filesystem) - - fstabs << fs.fstab - crypttabs << fs.crypttab - end - - fstabs.compact! - crypttabs.compact! - - [fstabs, crypttabs] + # @param disks [Array] + # @return [Array] + def windows_filesystems(*disks) + filesystems_collection(*disks).select(&:windows_system?) end - # Filesystems that could contain a Linux installation + # All filesystems that could contain Linux system from the given disks # - # @return [Array] - def suitable_root_filesystems - devicegraph.filesystems.select { |f| f.type.root_ok? } - end - - def release_name(filesystem) - fs = ExistingFilesystem.new(filesystem) - fs.release_name + # Note that filesystems over LVM LVs are not included. + # + # @see #filesystems_collection + # + # @param disks [Array] + # @return [Array] + def linux_suitable_filesystems(*disks) + filesystems_collection(*disks).select(&:root_suitable?) end - # Finds devices (disk devices and software RAIDs) that are suitable for installing Linux + # All filesystems that could contain a Linux system # - # From fate#326573 on, software RAIDs with partition table or without children are also - # considered as valid candidates. + # Note that {#linux_suitable_filesystems} does not take into account filesystems over a LVM LV. # - # @return [Array] candidate devices (disk devices and software RAIDs matching the - # conditions explained above) - def find_candidate_disks - find_candidate_software_raids + find_candidate_disk_devices + # @return [Array] + def all_linux_suitable_filesystems + @all_linux_suitable_filesystems ||= devicegraph.filesystems.select(&:root_suitable?) end # Finds software RAIDs that are considered valid candidates for a Linux installation @@ -299,8 +246,8 @@ def find_candidate_disks # either, have a partition table or do not have children. # # @return [Array] - def find_candidate_software_raids - @find_candidate_software_raids ||= devicegraph.software_raids.select do |md| + def candidate_software_raids + devicegraph.software_raids.select do |md| (md.partition_table? || md.children.empty?) && candidate_disk?(md) end end @@ -310,8 +257,8 @@ def find_candidate_software_raids # Basically, all available disk devices except those that are part of a candidate software RAID. # # @return [Array] - def find_candidate_disk_devices - rejected_disk_devices = find_candidate_software_raids.map(&:ancestors).flatten + def candidate_disk_devices + rejected_disk_devices = candidate_software_raids.map(&:ancestors).flatten candidate_disk_devices = devicegraph.disk_devices.select { |d| candidate_disk?(d) } candidate_disk_devices - rejected_disk_devices @@ -319,9 +266,8 @@ def find_candidate_disk_devices # Checks whether a device can be used as candidate disk for installation # - # @note A device is candidate for installation if no filesystem belonging - # to the device is mounted and the device does not contain a repository - # for installation. + # A device is candidate for installation if no filesystem belonging to the device is mounted and the + # device does not contain a repository for installation. # # @param device [BlkDevice] # @return [Boolean] @@ -342,10 +288,9 @@ def contain_mounted_filesystem?(device) # All filesystems inside a device # - # @note The device could be directly formatted or the filesystem could belong - # to a partition inside the device. Moreover, when the device (on any of its - # partitions) is used as LVM PV, all filesystem inside the LVM VG are considered - # as belonging to the device. + # The device could be directly formatted or the filesystem could belong to a partition inside the + # device. Moreover, when the device (on any of its partitions) is used as LVM PV, all filesystem + # inside the LVM VG are considered as belonging to the device. # # @param device [BlkDevice] # @return [Array] @@ -411,9 +356,7 @@ def all_devices_from_device(device) # # @return [Array] def repositories_devices - return @repositories_devices if @repositories_devices - - @repositories_devices = local_repositories.map { |r| repository_devices(r) }.flatten + @repositories_devices ||= local_repositories.map { |r| repository_devices(r) }.flatten end # TODO: This method should be moved to Y2Packager::Repository class diff --git a/src/lib/y2storage/partitionable.rb b/src/lib/y2storage/partitionable.rb index 905733333d..77862459de 100644 --- a/src/lib/y2storage/partitionable.rb +++ b/src/lib/y2storage/partitionable.rb @@ -1,4 +1,4 @@ -# Copyright (c) [2017] SUSE LLC +# Copyright (c) [2017-2019] SUSE LLC # # All Rights Reserved. # @@ -197,17 +197,6 @@ def linux_system_partitions partitions_with_id(:linux_system) end - # Partitions that could potentially contain a MS Windows installation - # - # @see ParitionId.windows_system_ids - # - # @return [Array] - def possible_windows_partitions - # Sorting is not mandatory, but keeping the output stable looks like a - # sane practice. - partitions.select(&:suitable_for_windows?).sort_by(&:number) - end - # Size between MBR and first partition. # # @see PartitionTables::Msdos#mbr_gap diff --git a/test/y2storage/disk_analyzer_test.rb b/test/y2storage/disk_analyzer_test.rb index cdf36fd80a..922a9d06c7 100755 --- a/test/y2storage/disk_analyzer_test.rb +++ b/test/y2storage/disk_analyzer_test.rb @@ -1,5 +1,5 @@ #!/usr/bin/env rspec -# Copyright (c) [2016] SUSE LLC +# Copyright (c) [2016-2019] SUSE LLC # # All Rights Reserved. # @@ -31,6 +31,74 @@ fake_scenario(scenario) end + describe "#windows_system?" do + let(:scenario) { "double-windows-pc" } + + context "when the architecture is not supported by Windows" do + let(:architecture) { :ppc } + + it "returns false" do + expect(analyzer.windows_system?).to eq(false) + end + + it "does not inspect any filesystem" do + expect_any_instance_of(Y2Storage::Filesystems::Base).to_not receive(:windows_system?) + + analyzer.windows_system? + end + end + + context "when the architecture is supported by Windows" do + let(:architecture) { :x86_64 } + + context "and there is a partition with a Windows system" do + before do + allow(Y2Storage::FilesystemReader).to receive(:new).and_return(reader) + + allow(reader).to receive(:windows?).and_return(*windows) + end + + let(:reader) { double("Y2Storage::FilesystemReader").as_null_object } + + let(:windows) { [true] } + + it "returns true" do + expect(analyzer.windows_system?).to eq(true) + end + + context "when a Windows is found for a filesystem" do + before do + allow_any_instance_of(Y2Storage::Filesystems::Base) + .to receive(:windows_suitable?).and_return(true) + end + + let(:windows) { [true, false, false, false] } + + it "does not check the next filesystem" do + expect(Y2Storage::FilesystemReader).to receive(:new).once.and_return(reader) + + analyzer.windows_system? + end + end + + context "when a Windows is not found for a filesystem" do + before do + allow_any_instance_of(Y2Storage::Filesystems::Base) + .to receive(:windows_suitable?).and_return(true) + end + + let(:windows) { [false, false, true, true] } + + it "checks the next filesystem" do + expect(Y2Storage::FilesystemReader).to receive(:new).exactly(3).times.and_return(reader) + + analyzer.windows_system? + end + end + end + end + end + describe "#windows_partitions" do context "in a PC" do context "for disks with no Windows" do @@ -72,11 +140,43 @@ end end + describe "#linux_partitions" do + context "for a disk with no linux partitions" do + let(:scenario) { "windows-pc-gpt" } + + it "returns an empty array" do + expect(analyzer.linux_partitions("/dev/sda").empty?).to eq(true) + end + end + + context "for a disk with linux partitions" do + let(:scenario) { "mixed_disks" } + + it "returns a list of partitions" do + expect(analyzer.linux_partitions("/dev/sda")).to be_a(Array) + expect(analyzer.linux_partitions("/dev/sda")).to all(be_a(Y2Storage::Partition)) + end + + it "includes all linux partitions" do + expect(analyzer.linux_partitions("/dev/sda").map(&:basename)).to contain_exactly("sda2") + end + end + + context "when no disk is indicated" do + let(:scenario) { "mixed_disks" } + + it "includes all linux partitions from all disks" do + expect(analyzer.linux_partitions.map(&:basename)) + .to contain_exactly("sda2", "sdb1", "sdb2", "sdb3", "sdb5", "sdb6", "sdb7") + end + end + end + describe "#installed_systems" do before do allow_any_instance_of(::Storage::BlkFilesystem).to receive(:detect_content_info) .and_return(content_info) - allow_any_instance_of(Y2Storage::ExistingFilesystem).to receive(:release_name) + allow_any_instance_of(Y2Storage::Filesystems::Base).to receive(:release_name) .and_return release_name end @@ -123,52 +223,52 @@ describe "#fstabs" do before do - allow_any_instance_of(Y2Storage::ExistingFilesystem).to receive(:fstab) - .and_return(Y2Storage::Fstab.new) + allow_any_instance_of(Y2Storage::Filesystems::Base) + .to receive(:fstab).and_return(Y2Storage::Fstab.new) end - it "returns a list with all found fstab files" do + it "returns a list of fstab files" do fstabs = analyzer.fstabs expect(fstabs).to be_a(Array) expect(fstabs).to all(be_a(Y2Storage::Fstab)) end - it "tries to read a fstab file for each suitable root filesystem" do - expect(Y2Storage::ExistingFilesystem).to receive(:new).exactly(5).times.and_call_original - - analyzer.fstabs + it "return fstab files from all root suitable filesystems" do + expect(analyzer.fstabs.size).to eq(5) end it "does not try to read fstab files again in subsequent calls" do analyzer.fstabs - expect(Y2Storage::ExistingFilesystem).to_not receive(:new) + + expect(Y2Storage::Fstab).to_not receive(:new) + analyzer.fstabs end end describe "#crypttabs" do before do - allow_any_instance_of(Y2Storage::ExistingFilesystem).to receive(:crypttab) - .and_return(Y2Storage::Crypttab.new) + allow_any_instance_of(Y2Storage::Filesystems::Base) + .to receive(:crypttab).and_return(Y2Storage::Crypttab.new) end - it "returns a list with all found crypttab files" do + it "returns a list of crypttab files" do crypttabs = analyzer.crypttabs expect(crypttabs).to be_a(Array) expect(crypttabs).to all(be_a(Y2Storage::Crypttab)) end - it "tries to read a crypttab file for each suitable root filesystem" do - expect(Y2Storage::ExistingFilesystem).to receive(:new).exactly(5).times.and_call_original - - analyzer.crypttabs + it "return crypttab files from suitable root filesystems" do + expect(analyzer.crypttabs.size).to eq(5) end it "does not try to read crypttab files again in subsequent calls" do analyzer.crypttabs - expect(Y2Storage::ExistingFilesystem).to_not receive(:new) + + expect(Y2Storage::Crypttab).to_not receive(:new) + analyzer.crypttabs end end From cfc3158770c1dfec18071d84cf6b06590ec1e6b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Wed, 4 Dec 2019 12:14:27 +0000 Subject: [PATCH 4/6] Fix unit tests --- test/support/proposal_context.rb | 12 ++++++++---- test/y2storage/guided_proposal_test.rb | 4 ---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/support/proposal_context.rb b/test/support/proposal_context.rb index e4e6b96698..be5a999ccf 100755 --- a/test/support/proposal_context.rb +++ b/test/support/proposal_context.rb @@ -27,12 +27,16 @@ fake_scenario(scenario) allow(Y2Storage::DiskAnalyzer).to receive(:new).and_return disk_analyzer - allow(disk_analyzer).to receive(:windows_partition?) do |partition| - partition.filesystem && !!(partition.filesystem.label =~ /indows/) + + # NOTE: Original method Y2Storage::Filesystems::Base#windows_system? tries to mount the filesystem to + # check if it contains a Windows system. This behaviour is mocked here to avoid the mounting action. + # For unit tests using this context file, a filesystem is considered to contain a Windows system when + # it is labeled as "windows". + allow_any_instance_of(Y2Storage::Filesystems::Base).to receive(:windows_system?) do |fs| + /windows/.match?(fs.label.downcase) end - allow_any_instance_of(Y2Storage::Partition).to receive(:detect_resize_info) - .and_return(resize_info) + allow_any_instance_of(Y2Storage::Partition).to receive(:detect_resize_info).and_return(resize_info) allow(Yast::Arch).to receive(:x86_64).and_return(architecture == :x86) allow(Yast::Arch).to receive(:i386).and_return(architecture == :i386) diff --git a/test/y2storage/guided_proposal_test.rb b/test/y2storage/guided_proposal_test.rb index 83d141fc88..6b7904059d 100755 --- a/test/y2storage/guided_proposal_test.rb +++ b/test/y2storage/guided_proposal_test.rb @@ -173,7 +173,6 @@ context "when forced to create a small partition" do let(:scenario) { "empty_hard_disk_gpt_25GiB" } - let(:windows_partitions) { {} } let(:separate_home) { true } let(:lvm) { false } @@ -194,7 +193,6 @@ context "when installing in a multipath device" do let(:scenario) { "empty-dasd-and-multipath.xml" } - let(:windows_partitions) { {} } let(:separate_home) { true } let(:lvm) { false } @@ -238,7 +236,6 @@ context "when installing in a DM RAID" do let(:scenario) { "empty-dm_raids_no_sda.xml" } - let(:windows_partitions) { {} } let(:separate_home) { false } let(:lvm) { false } @@ -312,7 +309,6 @@ end let(:scenario) { "swaps" } - let(:windows_partitions) { {} } let(:all_volumes) do [ planned_vol(mount_point: "/", type: :ext4, min: 500.MiB, max: 500.MiB), From ea947e7f76ea5aa526e19c2a3ec2187487ae7110 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Wed, 4 Dec 2019 12:19:47 +0000 Subject: [PATCH 5/6] Update version and changelog --- package/yast2-storage-ng.changes | 6 ++++++ package/yast2-storage-ng.spec | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/package/yast2-storage-ng.changes b/package/yast2-storage-ng.changes index 73fae6bef6..3189bc4e77 100644 --- a/package/yast2-storage-ng.changes +++ b/package/yast2-storage-ng.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Dec 4 12:17:37 UTC 2019 - José Iván López González + +- Improve detection of Windows systems (related to bsc#1135341). +- 4.2.60 + ------------------------------------------------------------------- Wed Nov 27 13:53:57 UTC 2019 - Christopher Hofmann diff --git a/package/yast2-storage-ng.spec b/package/yast2-storage-ng.spec index ed976e571a..2d993dcd0e 100644 --- a/package/yast2-storage-ng.spec +++ b/package/yast2-storage-ng.spec @@ -16,7 +16,7 @@ # Name: yast2-storage-ng -Version: 4.2.59 +Version: 4.2.60 Release: 0 Summary: YaST2 - Storage Configuration License: GPL-2.0-only OR GPL-3.0-only From 0727e031493bb24d382ac459dcac71487cf22496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Thu, 5 Dec 2019 08:18:01 +0000 Subject: [PATCH 6/6] Several improvements --- src/lib/y2storage/disk_analyzer.rb | 4 ++-- src/lib/y2storage/filesystem_reader.rb | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/lib/y2storage/disk_analyzer.rb b/src/lib/y2storage/disk_analyzer.rb index da93e385ea..88f6a27081 100755 --- a/src/lib/y2storage/disk_analyzer.rb +++ b/src/lib/y2storage/disk_analyzer.rb @@ -106,7 +106,7 @@ def linux_systems(*disks) # All fstabs found in the system # - # Note that all filesystems are considered here, including filesystems over LVM LVs, see + # Note that all Linux filesystems are considered here, including filesystems over LVM LVs, see # {#all_linux_suitable_filesystems}. # # @return [Array] @@ -116,7 +116,7 @@ def fstabs # All crypttabs found in the system # - # Note that all filesystems are considered here, including filesystems over LVM LVs, see + # Note that all Linux filesystems are considered here, including filesystems over LVM LVs, see # {#all_linux_suitable_filesystems}. # # @return [Array] diff --git a/src/lib/y2storage/filesystem_reader.rb b/src/lib/y2storage/filesystem_reader.rb index 99d2474962..904c218e10 100644 --- a/src/lib/y2storage/filesystem_reader.rb +++ b/src/lib/y2storage/filesystem_reader.rb @@ -91,6 +91,8 @@ def crypttab crypttab: nil }.freeze + private_constant :FS_ATTRIBUTES + # Filesystem attributes # # @return [Hash] @@ -216,14 +218,14 @@ def check_rpi_boot # Mounts the filesystem # - # @see execute + # @see #execute # # @note libstorage-ng has a couple of ways for immediate mounting/unmounting devices, but # they cannot be easily used here. # # For example, BlkFilesystem::detect_content_info uses an internal EnsureMounted object. # EnsureMounted mounts a given filesystem during its construction and unmounts it during - # its destuction. In ruby there is no a clear way of calling the destructor of a binding + # its destruction. In ruby there is no a clear way of calling the destructor of a binding # object, so EnsureMounted cannot be used for a temporary mount to inspect the filesystem # content from YaST and then unmount it. # @@ -247,7 +249,8 @@ def mount # Unmounts the filesystem # - # @see mount + # @see #mount + # @see #execute # # @raise [RuntimeError] when the filesystem cannot be unmounted def umount