feat(storage): avoid reference to compound actions (#1410)

A hot-fix was implemeted by #1400
in order to avoid possible segmentation fault when generating the list
of storage actions.

This PR provides a better solution. It avoids references to
`CompoundAction` objects instead of keeping an unnecessary reference to
the source `Actiongraph` object.

See #1396

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

@@ -160,7 +160,7 @@ def deprecated_system
         def read_actions
           backend.actions.map do |action|
             {
-              "Device" => action.device.sid,
+              "Device" => action.device_sid,
               "Text"   => action.text,
               "Subvol" => action.on_btrfs_subvolume?,
               "Delete" => action.delete?,

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

@@ -85,7 +85,7 @@ def actions
         #   * "Resize" [Boolean]
         def to_dbus_action(action)
           {
-            "Device" => action.device.sid,
+            "Device" => action.device_sid,
             "Text"   => action.text,
             "Subvol" => action.on_btrfs_subvolume?,
             "Delete" => action.delete?,

service/lib/agama/storage/action.rb

@@ -23,49 +23,51 @@ module Agama
   module Storage
     # Represents an action to perform in the storage devices.
     class Action
-      # @param action [Y2Storage::CompoundAction]
-      # @param system_graph [Y2Storage::Devicegraph]
-      def initialize(action, system_graph)
-        @action = action
-        @system_graph = system_graph
-      end
-
-      # Affected device
+      # SID of the affected device
       #
-      # @return [Y2Storage::Device]
-      def device
-        action.target_device
-      end
+      # @return [Integer]
+      attr_reader :device_sid
 
       # Text describing the action.
       #
       # @return [String]
-      def text
-        action.sentence
+      attr_reader :text
+
+      # @note Do not keep a reference to the compound action. Accessing to the compound action could
+      #   raise a segmentation fault if the source actiongraph object was killed by the ruby GC.
+      #
+      # See https://github.com/openSUSE/agama/issues/1396.
+      #
+      # @param action [Y2Storage::CompoundAction]
+      # @param system_graph [Y2Storage::Devicegraph]
+      def initialize(action, system_graph)
+        @system_graph = system_graph
+        @device_sid = action.target_device.sid
+        @text = action.sentence
+        @delete = action.delete?
+        @resize = resize_action?(action.target_device, system_graph)
+        @on_btrfs_subvolume = action.device_is?(:btrfs_subvolume)
       end
 
       # Whether the action affects to a Btrfs subvolume.
       #
       # @return [Boolean]
       def on_btrfs_subvolume?
-        action.device_is?(:btrfs_subvolume)
+        @on_btrfs_subvolume
       end
 
       # Whether the action deletes the device.
       #
       # @return [Boolean]
       def delete?
-        action.delete?
+        @delete
       end
 
       # Whether the action resizes the device.
       #
       # @return [Boolean]
       def resize?
-        return false unless device.exists_in_devicegraph?(system_graph)
-        return false unless device.respond_to?(:size)
-
-        system_graph.find_device(device.sid).size != device.size
+        @resize
       end
 
     private
@@ -75,6 +77,17 @@ def resize?
 
       # @return [Y2Storage::Devicegraph]
       attr_reader :system_graph
+
+      # @param device [Y2Storage::Device]
+      # @param system_graph [Y2Storage::Devicegraph]
+      #
+      # @return [Boolean]
+      def resize_action?(device, system_graph)
+        return false unless device.exists_in_devicegraph?(system_graph)
+        return false unless device.respond_to?(:size)
+
+        system_graph.find_device(device.sid).size != device.size
+      end
     end
   end
 end

service/lib/agama/storage/actions_generator.rb

@@ -29,10 +29,7 @@ class ActionsGenerator
       # param target_graph [Y2Storage::Devicegraph]
       def initialize(system_graph, target_graph)
         @system_graph = system_graph
-        # It is important to keep a reference to the actiongraph. Otherwise, the gargabe collector
-        # could kill the actiongraph, leaving the compound actions orphan.
-        # See https://github.com/openSUSE/agama/issues/1377.
-        @actiongraph = target_graph.actiongraph
+        @target_graph = target_graph
       end
 
       # All actions properly sorted.
@@ -47,8 +44,8 @@ def generate
       # @return [Y2Storage::Devicegraph]
       attr_reader :system_graph
 
-      # @return [Y2Storage::Actiongraph]
-      attr_reader :actiongraph
+      # @return [Y2Storage::Devicegraph]
+      attr_reader :target_graph
 
       # Sorted main actions (everything except subvolume actions).
       #
@@ -70,7 +67,7 @@ def subvolume_actions
       #
       # @return [Array<Action>]
       def actions
-        @actions ||= actiongraph.compound_actions.map do |action|
+        @actions ||= target_graph.actiongraph.compound_actions.map do |action|
           Action.new(action, system_graph)
         end
       end

service/lib/agama/storage/manager.rb

@@ -165,32 +165,13 @@ def software
 
       # Storage actions.
       #
-      # @return [Array<Y2Storage::CompoundAction>]
+      # @return [Array<Action>]
       def actions
         return [] unless Y2Storage::StorageManager.instance.probed?
 
         probed = Y2Storage::StorageManager.instance.probed
         staging = Y2Storage::StorageManager.instance.staging
-        # FIXME: This is a hot-fix to avoid segmentation fault in the actions, see
-        #   https://github.com/openSUSE/agama/issues/1396.
-        #
-        #   Source of the problem:
-        #   * An actiongraph is generated from the target devicegraph.
-        #   * The list of compound actions is recovered from the actiongraph.
-        #   * No refrence to the actiongraph is kept, so the object is a candidate to be cleaned by
-        #     the ruby GC.
-        #   * Accessing to the generated actions raises a segmentation fault if the actiongraph was
-        #     cleaned.
-        #
-        #   There was a previous attempt of fixing the issue by keeping a reference to the
-        #   actiongraph in the ActionsGenerator object. But that solution is not enough because
-        #   the ActionGenerator object is also cleaned up.
-        #
-        #   As a hot-fix, the generator is kept in an instance variable to avoid the GC to kill it.
-        #   A better solution is needed, for example, by avoiding to store an instance of a compound
-        #   action in the Action object.
-        @generator = ActionsGenerator.new(probed, staging)
-        @generator.generate
+        ActionsGenerator.new(probed, staging).generate
       end
 
       # Changes the service's locale

service/lib/agama/storage/proposal.rb

@@ -105,7 +105,7 @@ def calculate_autoyast(partitioning)
 
       # Storage actions.
       #
-      # @return [Array<Y2Storage::CompoundAction>]
+      # @return [Array<Action>]
       def actions
         return [] unless proposal&.devices
 

service/package/rubygem-agama-yast.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Fri Jun 28 11:57:39 UTC 2024 - José Iván López González <[email protected]>
+
+- Proper solution to avoid error in storage actions
+  (gh#openSUSE/agama#1410).
+
 -------------------------------------------------------------------
 Thu Jun 27 13:22:06 UTC 2024 - Imobach Gonzalez Sosa <[email protected]>
 

service/test/agama/dbus/storage/manager_test.rb

@@ -119,7 +119,7 @@
       let(:action1) do
         instance_double(Agama::Storage::Action,
           text:                "test1",
-          device:              device1,
+          device_sid:          1,
           on_btrfs_subvolume?: false,
           delete?:             false,
           resize?:             false)
@@ -128,7 +128,7 @@
       let(:action2) do
         instance_double(Agama::Storage::Action,
           text:                "test2",
-          device:              device2,
+          device_sid:          2,
           on_btrfs_subvolume?: false,
           delete?:             true,
           resize?:             false)
@@ -137,7 +137,7 @@
       let(:action3) do
         instance_double(Agama::Storage::Action,
           text:                "test3",
-          device:              device3,
+          device_sid:          3,
           on_btrfs_subvolume?: false,
           delete?:             false,
           resize?:             true)
@@ -146,17 +146,12 @@
       let(:action4) do
         instance_double(Agama::Storage::Action,
           text:                "test4",
-          device:              device4,
+          device_sid:          4,
           on_btrfs_subvolume?: true,
           delete?:             false,
           resize?:             false)
       end
 
-      let(:device1) { instance_double(Y2Storage::Device, sid: 1) }
-      let(:device2) { instance_double(Y2Storage::Device, sid: 2) }
-      let(:device3) { instance_double(Y2Storage::Device, sid: 3) }
-      let(:device4) { instance_double(Y2Storage::Device, sid: 4) }
-
       it "returns a list with a hash for each action" do
         expect(subject.actions.size).to eq(4)
         expect(subject.actions).to all(be_a(Hash))

service/test/agama/dbus/storage/proposal_test.rb

@@ -114,7 +114,7 @@
       let(:action1) do
         instance_double(Agama::Storage::Action,
           text:                "test1",
-          device:              device1,
+          device_sid:          1,
           on_btrfs_subvolume?: false,
           delete?:             false,
           resize?:             false)
@@ -123,7 +123,7 @@
       let(:action2) do
         instance_double(Agama::Storage::Action,
           text:                "test2",
-          device:              device2,
+          device_sid:          2,
           on_btrfs_subvolume?: false,
           delete?:             true,
           resize?:             false)
@@ -132,7 +132,7 @@
       let(:action3) do
         instance_double(Agama::Storage::Action,
           text:                "test3",
-          device:              device3,
+          device_sid:          3,
           on_btrfs_subvolume?: false,
           delete?:             false,
           resize?:             true)
@@ -141,17 +141,12 @@
       let(:action4) do
         instance_double(Agama::Storage::Action,
           text:                "test4",
-          device:              device4,
+          device_sid:          4,
           on_btrfs_subvolume?: true,
           delete?:             false,
           resize?:             false)
       end
 
-      let(:device1) { instance_double(Y2Storage::Device, sid: 1) }
-      let(:device2) { instance_double(Y2Storage::Device, sid: 2) }
-      let(:device3) { instance_double(Y2Storage::Device, sid: 3) }
-      let(:device4) { instance_double(Y2Storage::Device, sid: 4) }
-
       it "returns a list with a hash for each action" do
         expect(subject.actions.size).to eq(4)
         expect(subject.actions).to all(be_a(Hash))

service/test/agama/storage/action_test.rb

@@ -59,17 +59,17 @@
     described_class.new(action, system_graph)
   end
 
-  describe "#device" do
-    it "returns the affected device" do
+  describe "#device_sid" do
+    it "returns the SID of the affected device" do
       sda2 = system_graph.find_by_name("/dev/sda2")
-      expect(sda2_action.device.sid).to eq(sda2.sid)
+      expect(sda2_action.device_sid).to eq(sda2.sid)
 
       home_subvol = sda2
         .filesystem
         .btrfs_subvolumes
         .find { |s| s.path == "home" }
 
-      expect(subvol_action.device.sid).to eq(home_subvol.sid)
+      expect(subvol_action.device_sid).to eq(home_subvol.sid)
     end
   end