Fix for bsc#1084213 and bsc#1084261 #567
Conversation
| @@ -418,6 +418,8 @@ def remove_shadowed_subvols(planned_devices) | |||
| subvols_added = | |||
| device.respond_to?(:mount_point) && device.mount_point == "/" && @default_subvolumes_used | |||
|
|
|||
| # Use #dup to avoid bsc#1084213 | |||
There was a problem hiding this comment.
I would dup the list just when it is assigned from the settings:
https://github.com/ancorgs/yast-storage-ng/blob/b59dafb1b2e7c1d58d35e914907ea7fe7b0ef7dc/src/lib/y2storage/proposal/autoinst_devices_planner.rb#L233
This would avoid possible similar errors in other places.
There was a problem hiding this comment.
There are at least three assignations of that. So duping all of them looks wrong.
At most, I would define a setter. Like
module Planned::CanBeFormatted
def subvolumes=(value)
@subvolumes = value.dup
end
endThat was indeed my first option, but I somehow though it would have been more surprising. Less rubyist, sort to say.
I don't care much, either solution (the one on this PR or redefining the setter) are acceptable to me.
There was a problem hiding this comment.
So:
- Solution 1: the one currently on this PR. It's correct but @joseivanlopez pointed that we can make the same error in the future in another part.
- Solution 2: redefining the setter. It's also correct and doesn't have the mentioned problem, although it has slightly surprising implications (from the OOP rubyist POV).
- Solution 3: a
dupon each assignation as suggested by @joseivanlopez. I don't like it. Very error prone.
I discussed offline with @imobachgs and he would go for 2. Since 2 is also pretty aligned with @joseivanlopez's concerns, I will switch from 1 to 2.
| @@ -105,10 +105,13 @@ def remove_shadowed_subvolumes(planned_devices) | |||
| planned_devices.each do |device| | |||
| next unless device.respond_to?(:subvolumes) | |||
|
|
|||
| # Use #dup to avoid bsc#1084213 | |||
There was a problem hiding this comment.
|
Verified manually that this fixes the reported bug. |
ancorgs
changed the title
|
New solution pushed with |
|
New solution also tested manually |
| # # most common ruby behavior. | ||
| # my_list.first.path = "changed" # This change affects planned.subvolumes | ||
| # # because the object is also in that collection (not a deep copy). | ||
| # |
There was a problem hiding this comment.
Why not a deep copy? Having a mixed behavior could be confusing. I would expect that if I obtain a new collection, that new collection is totally independent to the original one. Moreover, we should avoid any unwanted change in the original settings when we are configuring the planned devices.
There was a problem hiding this comment.
Well, a deep copy would be even more unexpected. When I pass a list of objects I'm indeed intent to pass those objects, not a copy of them. Two objects that are not the same object but represent the same thing are a constant source of bugs (they don't work with include?, ==, etc.)
First step to fix https://trello.com/c/MSBg5L2S/304-sles15-p1-1084213-storage-ng-home-subvolume-not-created-if-user-opts-to-not-have-a-home-partition