From a4a082e70efa848bd737076b47e4d11ec7ec9aab Mon Sep 17 00:00:00 2001 From: Jeff Bonhag Date: Wed, 17 Jun 2020 16:20:35 -0400 Subject: [PATCH] Prevent multiple calls to #read_storage_controllers --- .../providers/virtualbox/cap/cleanup_disks.rb | 2 +- .../providers/virtualbox/cap/configure_disks.rb | 15 ++++++++++++--- .../providers/virtualbox/driver/version_5_0.rb | 16 ---------------- .../virtualbox/cap/cleanup_disks_test.rb | 14 +++++++++++++- .../virtualbox/cap/configure_disks_test.rb | 3 --- 5 files changed, 26 insertions(+), 24 deletions(-) diff --git a/plugins/providers/virtualbox/cap/cleanup_disks.rb b/plugins/providers/virtualbox/cap/cleanup_disks.rb index 04d506eb4..b03883bd2 100644 --- a/plugins/providers/virtualbox/cap/cleanup_disks.rb +++ b/plugins/providers/virtualbox/cap/cleanup_disks.rb @@ -32,7 +32,7 @@ module VagrantPlugins else primary_controller = storage_controllers.detect { |c| c.storage_bus == "SATA" } if primary_controller.nil? - # raise exception + raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, storage_bus: "SATA" end end diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index 3d2d8d57e..d354423dc 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -43,7 +43,11 @@ module VagrantPlugins else disks_defined = defined_disks.select { |d| d.type == :disk } if disks_defined.any? - disk_controller = machine.provider.driver.get_controller("SATA") + disk_controller = storage_controllers.detect { |c| c.storage_bus == "SATA" } + if disk_controller.nil? + raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, storage_bus: "SATA" + end + if (disks_defined.any? { |d| d.primary } && disks_defined.size > disk_controller.limit) || disks_defined.size > disk_controller.limit - 1 raise Vagrant::Errors::VirtualBoxDisksDefinedExceedLimit, @@ -54,7 +58,11 @@ module VagrantPlugins dvds_defined = defined_disks.select { |d| d.type == :dvd } if dvds_defined.any? - dvd_controller = machine.provider.driver.get_controller("IDE") + dvd_controller = storage_controllers.detect { |c| c.storage_bus == "IDE" } + if dvd_controller.nil? + raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, storage_bus: "IDE" + end + if disks_defined.size > dvd_controller.limit raise Vagrant::Errors::VirtualBoxDisksDefinedExceedLimit, limit: dvd_controller.limit, @@ -174,7 +182,8 @@ module VagrantPlugins machine.provider.driver.attach_disk(controller.name, port, device, "dvddrive", dvd.file) # Refresh the controller information - controller = machine.provider.driver.get_controller(controller.storage_bus) + storage_controllers = machine.provider.driver.read_storage_controllers + controller = storage_controllers.detect { |c| c.name == controller.name } attachment = controller.attachments.detect { |a| a[:port] == port && a[:device] == device } if attachment diff --git a/plugins/providers/virtualbox/driver/version_5_0.rb b/plugins/providers/virtualbox/driver/version_5_0.rb index 7ba91516a..78d5d0384 100644 --- a/plugins/providers/virtualbox/driver/version_5_0.rb +++ b/plugins/providers/virtualbox/driver/version_5_0.rb @@ -954,22 +954,6 @@ module VagrantPlugins end end - # Get the controller that uses the specified storage bus. - # - # A VirtualBoxDisksControllerNotFound error is raised if a compatible - # storage controller cannot be found. - # - # @param [String] storage_bus - for example, 'IDE' or 'SATA' - # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] controller - def get_controller(storage_bus) - storage_controllers = read_storage_controllers - controller = storage_controllers.detect { |c| c.storage_bus == storage_bus } - if controller.nil? - raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, storage_bus: storage_bus - end - controller - end - protected def valid_ip_address?(ip) diff --git a/test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb b/test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb index 1cc11f061..2ee853c13 100644 --- a/test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb @@ -35,10 +35,12 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::CleanupDisks do let(:controller) { double("controller", name: "controller", limit: 30, storage_bus: "SATA", maxportcount: 30) } + let(:storage_controllers) { [controller] } + before do allow(Vagrant::Util::Experimental).to receive(:feature_enabled?).and_return(true) allow(controller).to receive(:attachments).and_return(attachments) - allow(driver).to receive(:read_storage_controllers).and_return([controller]) + allow(driver).to receive(:read_storage_controllers).and_return(storage_controllers) end describe "#cleanup_disks" do @@ -100,6 +102,16 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::CleanupDisks do subject.handle_cleanup_disk(machine, defined_disks, disk_meta_file[:disk]) end end + + context "with multiple storage controllers" do + let(:storage_controllers) { [ double("controller1", storage_bus: "IDE"), + double("controller2", storage_bus: "SCSI") ] } + + it "assumes that disks will be attached to the SATA controller" do + expect { subject.handle_cleanup_disk(machine, defined_disks, disk_meta_file[:disk]) }. + to raise_error(Vagrant::Errors::VirtualBoxDisksControllerNotFound) + end + end end describe "#handle_cleanup_dvd" do diff --git a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb index 497516c06..b99c4bc4f 100644 --- a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb @@ -69,7 +69,6 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do allow(Vagrant::Util::Experimental).to receive(:feature_enabled?).and_return(true) allow(controller).to receive(:attachments).and_return(attachments) allow(driver).to receive(:read_storage_controllers).and_return([controller]) - allow(driver).to receive(:get_controller).with(controller.storage_bus).and_return(controller) end describe "#configure_disks" do @@ -120,8 +119,6 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do before do allow(driver).to receive(:read_storage_controllers).and_return([controller1, controller2]) - allow(driver).to receive(:get_controller).with(controller1.storage_bus).and_return(controller1) - allow(driver).to receive(:get_controller).with(controller2.storage_bus).and_return(controller2) end it "attaches disks to the SATA controller" do