improve code & add Btrfs#canonical_subvolume_name

src/lib/y2storage/filesystems/btrfs.rb

@@ -80,6 +80,20 @@ def supports_btrfs_subvolumes?
         true
       end
 
+      # Convert path to canonical form.
+      #
+      # That is, a single slash between elements. No final slash except for "/".
+      #
+      # @param path [String] subvolume path
+      #
+      # @return [String] sanitized subvolume path
+      #
+      def canonical_subvolume_name(path)
+        path = path.squeeze("/")
+        path = path.chomp("/") if path != "/"
+        path
+      end
+
       # Check if a subvolume can be created.
       #
       # It can always be created if we're going to create the whole file
@@ -106,7 +120,8 @@ def subvolume_can_be_created?(path)
       # @return [Array<BtrfsSubvolume>]
       #
       def subvolume_descendants(path)
-        path += "/"
+        path = canonical_subvolume_name(path)
+        path += "/" unless path.end_with?("/")
         btrfs_subvolumes.find_all { |sv| sv.path.start_with?(path) }
       end
 
@@ -149,7 +164,9 @@ def delete_btrfs_subvolume(devicegraph, path)
         devicegraph.remove_btrfs_subvolume(subvolume)
       end
 
-      # Create a new btrfs subvolume for the filesystem
+      # Creates a new btrfs subvolume for the filesystem
+      #
+      # If the subvolume already exists, returns it.
       #
       # @note The subvolume mount point is generated from the filesystem mount point
       # and the subvolume path.
@@ -162,6 +179,8 @@ def delete_btrfs_subvolume(devicegraph, path)
       # @return [BtrfsSubvolume, nil] new subvolume
       #
       def create_btrfs_subvolume(path, nocow)
+        path = canonical_subvolume_name(path)
+
         subvolume = find_btrfs_subvolume_by_path(path)
         return subvolume if subvolume
 
@@ -453,7 +472,7 @@ def subvolume_descendants_exist?(path)
       #
       # @return [BtrfsSubvolume] parent subvolume
       #
-      def best_match(path)
+      def suitable_parent_subvolume(path)
         while path.include?("/")
           path = path.gsub(/\/[^\/]*$/, "")
           subvolume = find_btrfs_subvolume_by_path(path)
@@ -514,7 +533,7 @@ def create_btrfs_subvolume_full_rebuild(path, nocow)
       # @return [BtrfsSubvolume] new subvolume
       #
       def create_btrfs_subvolume_nochecks(path, nocow)
-        parent_subvolume = best_match(path)
+        parent_subvolume = suitable_parent_subvolume(path)
 
         log.info "creating subvolume #{path} at #{parent_subvolume.path}"
 

test/y2storage/filesystems/btrfs_test.rb

@@ -230,15 +230,33 @@
     end
 
     context "when the filesystem already exists" do
+      let(:result) {}
+
       before do
         allow(filesystem).to receive(:exists_in_probed?) .and_return(true)
       end
 
       it "can not insert a subvolume into an existing hierarchy" do
         filesystem.create_btrfs_subvolume(path2, nocow)
-        filesystem.create_btrfs_subvolume(path3, nocow)
+        result = filesystem.create_btrfs_subvolume(path3, nocow)
+        expect(result).to be_nil
         expect(filesystem.btrfs_subvolumes.map(&:path)).to_not include(path3)
       end
+
+      it "and returns nil" do
+        expect(result).to be_nil
+      end
+    end
+  end
+
+  describe "#canonical_subvolume_name" do
+    let(:devicegraph) { Y2Storage::StorageManager.instance.staging }
+
+    it "converts subvolume name into the canonical form" do
+      expect(filesystem.canonical_subvolume_name("@/foo")).to eq "@/foo"
+      expect(filesystem.canonical_subvolume_name("@/foo/////bar//")).to eq "@/foo/bar"
+      expect(filesystem.canonical_subvolume_name("/")).to eq "/"
+      expect(filesystem.canonical_subvolume_name("")).to eq ""
     end
   end