Improvements from review

doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml

@@ -21,8 +21,8 @@
         TargetDevice            s
         TargetVG                s
         FsType                  s
-        MinSize                 t
-        MaxSize                 t (optinal, max size is considered as unlimited if omitted)
+        MinSize                 t (bytes)
+        MaxSize                 t (bytes. Optinal, max size is considered as unlimited if omitted)
         AutoSize                b
         Snapshots               b
         Outline                 a{sv}

doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml

@@ -19,8 +19,8 @@
         TargetDevice            s
         TargetVG                s
         FsType                  s
-        MinSize                 t
-        MaxSize                 t (optinal, max size is considered as unlimited if omitted)
+        MinSize                 t (bytes)
+        MaxSize                 t (bytes. Optinal, max size is considered as unlimited if omitted)
         AutoSize                b
         Snapshots               b
         Outline                 a{sv}

service/etc/agama.yaml

@@ -41,7 +41,7 @@ ALP-Dolomite:
       - package: device-mapper # Apparently needed if devices at /dev/mapper are used at boot (eg. FDE)
       - package: fde-tools # Needed for FDE with TPM, hardcoded here temporarily
         archs: aarch64, x86_64
-      - package: libtss2-tcti-device0 # Same than fde-tools
+      - package: libtss2-tcti-device0
       - package: ppc64-diag # Needed for hardware-based installations
         archs: ppc64
     optional_packages: null
@@ -255,7 +255,7 @@ Leap16:
       - package: device-mapper # Apparently needed if devices at /dev/mapper are used at boot (eg. FDE)
       - package: fde-tools # Needed for FDE with TPM, hardcoded here temporarily
         archs: aarch64, x86_64
-      - package: libtss2-tcti-device0 # Same than fde-tools
+      - package: libtss2-tcti-device0
     optional_packages: null
     base_product: Leap16
 

service/lib/agama/dbus/storage/manager.rb

@@ -145,9 +145,13 @@ def default_volume(mount_path)
         # @param dbus_settings [Hash]
         # @return [Integer] 0 success; 1 error
         def calculate_proposal(dbus_settings)
-          logger.info("Calculating storage proposal from D-Bus settings: #{dbus_settings}")
-
           settings = ProposalSettingsConversion.from_dbus(dbus_settings, config: config)
+          logger.info(
+            "Calculating storage proposal from D-Bus.\n " \
+            "D-Bus settings: #{dbus_settings}\n" \
+            "Agama settings: #{settings}"
+          )
+
           success = proposal.calculate(settings)
 
           success ? 0 : 1

service/lib/agama/dbus/storage/proposal.rb

@@ -65,7 +65,7 @@ def initialize(backend, logger)
 
         # Device used as boot device by the storage proposal
         #
-        # @return [String]
+        # @return [String] Empty string if no device has been selected yet.
         def boot_device
           dbus_settings.fetch("BootDevice", "")
         end

service/lib/agama/dbus/storage/proposal_settings_conversion/to_dbus.rb

@@ -38,7 +38,7 @@ def initialize(settings)
           def convert # rubocop:disable Metrics/AbcSize
             {
               "BootDevice"             => settings.boot_device.to_s,
-              "LVM"                    => settings.lvm.enabled,
+              "LVM"                    => settings.lvm.enabled?,
               "SystemVGDevices"        => settings.lvm.system_vg_devices,
               "EncryptionPassword"     => settings.encryption.password.to_s,
               "EncryptionMethod"       => settings.encryption.method.id.to_s,

service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb

@@ -104,8 +104,10 @@ def target_vg_conversion(target, value)
           # @param target [Agama::Storage::Volume]
           # @param value [String]
           def fs_type_conversion(target, value)
+            downcase_value = value.downcase
+
             fs_type = target.outline.filesystems.find do |type|
-              type.to_human_string.downcase == value.downcase
+              type.to_human_string.downcase == downcase_value
             end
 
             return unless fs_type
@@ -114,13 +116,13 @@ def fs_type_conversion(target, value)
           end
 
           # @param target [Agama::Storage::Volume]
-          # @param value [Integer]
+          # @param value [Integer] Size in bytes.
           def min_size_conversion(target, value)
             target.min_size = Y2Storage::DiskSize.new(value)
           end
 
           # @param target [Agama::Storage::Volume]
-          # @param value [Integer]
+          # @param value [Integer] Size in bytes.
           def max_size_conversion(target, value)
             target.max_size = Y2Storage::DiskSize.new(value)
           end

service/lib/agama/storage/encryption_settings.rb

@@ -36,7 +36,9 @@ class EncryptionSettings
       # @return [Y2Storage::EncryptionMethod::Base]
       attr_accessor :method
 
-      # @return [Y2Storage::PbkdFunction, nil]
+      # PBKD function to use for LUKS2
+      #
+      # @return [Y2Storage::PbkdFunction, nil] Can be nil if using LUKS1.
       attr_accessor :pbkd_function
 
       def initialize

service/lib/agama/storage/lvm_settings.rb

@@ -27,6 +27,7 @@ class LvmSettings
       #
       # @return [Boolean]
       attr_accessor :enabled
+      alias_method :enabled?, :enabled
 
       # Devices to use for the system LVM volume group
       #
@@ -37,11 +38,6 @@ def initialize
         @enabled = false
         @system_vg_devices = []
       end
-
-      # @return [Boolean]
-      def enabled?
-        !!@enabled
-      end
     end
   end
 end

service/lib/agama/storage/proposal.rb

@@ -101,7 +101,8 @@ def calculate(settings)
       # Note that this settings might differ from the {#original_settings}. For example, the sizes
       # of some volumes could be adjusted if auto size is set.
       #
-      # @return [ProposalSettings, nil]
+      # @return [ProposalSettings, nil] nil if no proposal has been calculated yet or the proposal
+      #   was invalidated.
       def settings
         return nil unless calculated?
 

service/lib/agama/storage/proposal_settings.rb

@@ -45,7 +45,7 @@ class ProposalSettings
       # Device name of the disk that will be used for booting the system and also to allocate all
       # the partitions, except those that have been explicitly assigned to other disk(s).
       #
-      # @return [String, nil]
+      # @return [String, nil] nil if no device has been selected yet.
       attr_accessor :boot_device
 
       # Set of volumes to create

service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb

@@ -136,13 +136,13 @@ def fallbacks_conversion(target)
           end
         end
 
-        # @param mount_path [String]
+        # @param mount_path [String, nil] nil if not found
         def find_min_size_fallback(mount_path)
           volume = settings.volumes.find { |v| v.min_size_fallback_for.include?(mount_path) }
           volume&.mount_path
         end
 
-        # @param mount_path [String]
+        # @param mount_path [String, nil] nil if not found
         def find_max_size_fallback(mount_path)
           volume = settings.volumes.find { |v| v.max_size_fallback_for.include?(mount_path) }
           volume&.mount_path
@@ -167,7 +167,7 @@ def all_devices
         # @return [String, Array<String>]
         def device_or_partitions(device)
           partitions = partitions(device)
-          partitions.any? ? partitions : device
+          partitions.empty? ? device : partitions
         end
 
         # @param device [String]

service/lib/agama/storage/volume_conversion/from_y2storage.rb

@@ -61,8 +61,15 @@ def convert
 
         # @param target [Agama::Storage::Volume]
         def sizes_conversion(target)
-          target.auto_size = !(spec.ignore_fallback_sizes? && spec.ignore_snapshots_sizes?)
+          target.auto_size = !spec.ignore_fallback_sizes? || !spec.ignore_snapshots_sizes?
 
+          # The volume specification contains the min and max sizes for the volume. But the final
+          # range of sizes used by the Y2Storage proposal depends on the fallback sizes (if this
+          # volume is fallback for other volume) and the size for snapshots (if snapshots is
+          # active). The planned device contains the real range of sizes used by the proposal.
+          #
+          # From Agama point of view, this is the way of recovering the range of sizes used by
+          # Y2Storage when a volume is set to have auto size.
           planned = planned_device_for(spec.mount_point)
           target.min_size = planned&.min || spec.min_size
           target.max_size = planned&.max || spec.max_size

service/lib/agama/storage/volume_conversion/to_y2storage.rb

@@ -65,6 +65,11 @@ def sizes_conversion(target)
           target.ignore_fallback_sizes = !auto
           target.ignore_snapshots_sizes = !auto
 
+          # The range of sizes is defined by the volume outline in case of auto size (mix and max
+          # sizes cannot be configured if auto size is set).
+          # And note that the inal range of sizes used by the Y2Storage proposal is calculated by
+          # Y2Storage according the range configured here and other sizes like fallback sizes or
+          # the size for snapshots.
           target.min_size = auto ? volume.outline.base_min_size : volume.min_size
           target.max_size = auto ? volume.outline.base_max_size : volume.max_size
         end

service/lib/agama/storage/volume_outline.rb

@@ -41,13 +41,13 @@ class VolumeOutline
       attr_accessor :filesystems
 
       # Base value to calculate the min size for the volume (if #auto_size is set to true
-      # for that final volume) or to use as default value (if #auto_size is false)
+      # for that final volume)
       #
       # @return [Y2Storage::DiskSize]
       attr_accessor :base_min_size
 
       # Base value to calculate the max size for the volume (if #auto_size is set to true
-      # for that final volume) or to use as default value (if #auto_size is false)
+      # for that final volume)
       #
       # @return [Y2Storage::DiskSize]
       attr_accessor :base_max_size
@@ -56,10 +56,22 @@ class VolumeOutline
       attr_accessor :adjust_by_ram
       alias_method :adjust_by_ram?, :adjust_by_ram
 
-      # @return [Array<String>] mount paths of other volumes
+      # Lists the mount paths of the volumes for which this volume is a min size fallback.
+      #
+      # Being a min size fallback means that the min size of the volume would be increased by the
+      # min size of other volumes, if any of that other volumes is not used for the proposal.
+      #
+      # For example, let's say the root volume is a fallback for the min size of /home and /var
+      # (root.min_size_fallback_for => ["/home", "/var"]). And a proposal is calculated with only
+      # root and /home. In that case, the min size of /var is added to the min size of root. The
+      # same would happen for /home if the proposal does not include it.
+      #
+      # @return [Array<String>]
       attr_accessor :min_size_fallback_for
 
-      # @return [Array<String>] mount paths of other volumes
+      # The same as {#min_size_fallback_for}, but for the max size of the volume.
+      #
+      # @return [Array<String>]
       attr_accessor :max_size_fallback_for
 
       # Whether snapshots option can be configured
@@ -70,12 +82,12 @@ class VolumeOutline
 
       # Size required for snapshots
       #
-      # @return [Y2Storage::DiskSize, nil]
+      # @return [Y2Storage::DiskSize, nil] nil if no extra size for snapshots.
       attr_accessor :snapshots_size
 
       # Percentage of space required for snapshots
       #
-      # @return [Integer, nil]
+      # @return [Integer, nil] nil if no extra size for snapshots.
       attr_accessor :snapshots_percentage
 
       def initialize