Use same list_all_networks and filter (#1638)

Ensure the same filtering for networks supported by vagrant-libvirt is
done for both driver and util by moving to call the same function with
filtering.

This avoids calls for the list of host devices from failing to parse
some networks that are not supported.

Fixes: #599

lib/vagrant-libvirt/action/create_network_interfaces.rb

@@ -61,7 +61,7 @@ module VagrantPlugins
             # We have slot for interface, fill it with interface configuration.
             adapters[free_slot] = options
             adapters[free_slot][:network_name] = interface_network(
-              env[:machine].provider.driver.connection.client, adapters[free_slot]
+              env[:machine].provider.driver, adapters[free_slot]
             )
           end
 
@@ -282,7 +282,7 @@ module VagrantPlugins
         end
 
         # Return network name according to interface options.
-        def interface_network(libvirt_client, options)
+        def interface_network(driver, options)
           # no need to get interface network for tcp tunnel config
           return 'tunnel_interface' if options.fetch(:tunnel_type, nil)
 
@@ -292,7 +292,7 @@ module VagrantPlugins
           end
 
           # Get list of all (active and inactive) Libvirt networks.
-          available_networks = libvirt_networks(libvirt_client)
+          available_networks = libvirt_networks(driver)
 
           return 'public' if options[:iface_type] == :public_network
 

lib/vagrant-libvirt/action/create_networks.rb

@@ -35,9 +35,7 @@ module VagrantPlugins
             # for VMs using sessions. It is likely that this should be done
             # to determine the correct virtual device for the management
             # network for sessions instead of assuming the default of virbr0.
-            @available_networks = libvirt_networks(
+            @available_networks = libvirt_networks(env[:machine].provider.driver)
-              env[:machine].provider.driver.system_connection
-            )
 
             @app.call(env)
             return
@@ -61,9 +59,7 @@ module VagrantPlugins
               # Get a list of all (active and inactive) Libvirt networks. This
               # list is used throughout this class and should be easier to
               # process than Libvirt API calls.
-              @available_networks = libvirt_networks(
+              @available_networks = libvirt_networks(env[:machine].provider.driver)
-                env[:machine].provider.driver.connection.client
-              )
 
               current_network = @available_networks.detect { |network| network[:name] == @options[:network_name] }
 

lib/vagrant-libvirt/driver.rb

@@ -197,6 +197,19 @@ module VagrantPlugins
         domain
       end
 
+      def list_all_networks
+        system_connection.list_all_networks.select do |net|
+          begin
+            net.bridge_name
+          rescue Libvirt::Error
+            # there does not appear to be a mechanism to determine the type of network, only by
+            # querying the attribute and catching the error is it possible to ignore unsupported.
+            @logger.debug "Ignoring #{net.name} as it does not support retrieval of bridge_name attribute"
+            next
+          end
+        end
+      end
+
       def host_devices
         @host_devices ||= begin
           cmd = []
@@ -216,7 +229,7 @@ module VagrantPlugins
           (
             info.map { |iface| iface['ifname'] } +
             connection.client.list_all_interfaces.map { |iface| iface.name } +
-            connection.client.list_all_networks.map { |net| net.bridge_name }
+            list_all_networks.map { |net| net.bridge_name }
           ).uniq.reject(&:empty?)
         end
       end
@@ -234,7 +247,7 @@ module VagrantPlugins
       def get_ipaddress_from_system(mac)
         ip_address = nil
 
-        system_connection.list_all_networks.each do |net|
+        list_all_networks.each do |net|
           leases = net.dhcp_leases(mac, 0)
           # Assume the lease expiring last is the current IP address
           ip_address = leases.max_by { |lse| lse['expirytime'] }['ipaddr'] unless leases.empty?

lib/vagrant-libvirt/util/network_util.rb

@@ -149,19 +149,11 @@ module VagrantPlugins
 
         # Return a list of all (active and inactive) Libvirt networks as a list
         # of hashes with their name, network address and status (active or not)
-        def libvirt_networks(libvirt_client)
+        def libvirt_networks(driver)
           libvirt_networks = []
 
           # Iterate over all (active and inactive) networks.
-          libvirt_client.list_all_networks.each do |libvirt_network|
+          driver.list_all_networks.each do |libvirt_network|
-            begin
-              bridge_name = libvirt_network.bridge_name
-            rescue Libvirt::Error
-              # there does not appear to be a mechanism to determine the type of network, only by
-              # querying the attribute and catching the error is it possible to ignore unsupported.
-              @logger.debug "Ignoring #{libvirt_network.name} as it does not support retrieval of bridge_name attribute"
-              next
-            end
 
             # Parse ip address and netmask from the network xml description.
             xml = Nokogiri::XML(libvirt_network.xml_desc)
@@ -189,7 +181,7 @@ module VagrantPlugins
               netmask:          netmask,
               network_address:  network_address,
               dhcp_enabled:     dhcp_enabled,
-              bridge_name:      bridge_name,
+              bridge_name:      libvirt_network.bridge_name,
               domain_name:      domain_name,
               created:          true,
               active:           libvirt_network.active?,

spec/unit/driver_spec.rb

@@ -187,7 +187,7 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
 
       context 'when qemu_use_session is enabled' do
         let(:system_connection) { double("system connection") }
-        let(:networks) { [instance_double(::Fog::Libvirt::Compute::Real)] }
+        let(:networks) { [instance_double(::Libvirt::Network)] }
         let(:dhcp_leases) {
           {
             "iface"      =>"virbr0",
@@ -207,8 +207,7 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
 
         it 'should retrieve the address via the system dhcp-leases API' do
           expect(domain).to receive(:mac).and_return("52:54:00:8b:dc:5f")
-          expect(subject).to receive(:system_connection).and_return(system_connection)
+          expect(subject).to receive(:list_all_networks).and_return(networks)
-          expect(system_connection).to receive(:list_all_networks).and_return(networks)
           expect(networks[0]).to receive(:dhcp_leases).and_return([dhcp_leases])
 
           expect(subject.get_ipaddress).to eq("192.168.122.43")
@@ -229,6 +228,43 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
     end
   end
 
+
+  describe '#list_all_networks' do
+    let(:vagrantfile_providerconfig) do
+      <<-EOF
+        libvirt.uri = "qemu:///system"
+      EOF
+    end
+
+    let(:libvirt_networks) { [
+      instance_double(::Libvirt::Network),
+      instance_double(::Libvirt::Network),
+      instance_double(::Libvirt::Network),
+    ] }
+
+    before do
+      allow(subject).to receive(:system_connection).and_return(libvirt_client)
+      expect(libvirt_client).to receive(:list_all_networks).and_return(libvirt_networks)
+    end
+
+    it 'should list networks' do
+      expect(libvirt_networks[0]).to receive(:bridge_name).and_return('')
+      expect(libvirt_networks[1]).to receive(:bridge_name).and_return('virbr0')
+      expect(libvirt_networks[2]).to receive(:bridge_name).and_return('virbr1')
+
+      expect(subject.list_all_networks).to eq(libvirt_networks)
+    end
+
+    it 'should skip networks missing bridge_name' do
+      expect(libvirt_networks[0]).to receive(:bridge_name).and_return('')
+      expect(libvirt_networks[1]).to receive(:bridge_name).and_raise(Libvirt::Error)
+      expect(libvirt_networks[1]).to receive(:name).and_return('bad_network')
+      expect(libvirt_networks[2]).to receive(:bridge_name).and_return('virbr1')
+
+      expect(subject.list_all_networks).to eq([libvirt_networks[0], libvirt_networks[2]])
+    end
+  end
+
   describe '#host_devices' do
     let(:vagrantfile_providerconfig) do
       <<-EOF
@@ -265,7 +301,7 @@ describe VagrantPlugins::ProviderLibvirt::Driver do
       end.and_return(Vagrant::Util::Subprocess::Result.new(exit_code=0, stdout=ip_link_show, stderr=''))
 
       expect(libvirt_client).to receive(:list_all_interfaces).and_return(libvirt_interfaces)
-      expect(libvirt_client).to receive(:list_all_networks).and_return(libvirt_networks)
+      expect(subject).to receive(:list_all_networks).and_return(libvirt_networks)
       expect(libvirt_interfaces[0]).to receive(:name).and_return('eth0')
       expect(libvirt_interfaces[1]).to receive(:name).and_return('virbr0')
       expect(libvirt_networks[0]).to receive(:bridge_name).and_return('')

spec/unit/util/network_util_spec.rb

@@ -45,23 +45,12 @@ describe 'VagrantPlugins::ProviderLibvirt::Util::NetworkUtil' do
   describe '#libvirt_networks' do
     let(:default_network) { create_libvirt_network('default') }
     let(:additional_network) { create_libvirt_network('vagrant-libvirt') }
-    let(:hostdev_network) { create_libvirt_network('hostdev', {:active? => false}) }
 
     it 'should retrieve the list of networks' do
       expect(logger).to_not receive(:debug)
-      expect(libvirt_client).to receive(:list_all_networks).and_return([default_network, additional_network])
+      expect(driver).to receive(:list_all_networks).and_return([default_network, additional_network])
 
-      expect(subject.libvirt_networks(libvirt_client)).to match_array([
+      expect(subject.libvirt_networks(driver)).to match_array([
-        hash_including(:name => 'default'),
-        hash_including(:name => 'vagrant-libvirt'),
-      ])
-    end
-
-    it 'should handle networks without bridge names' do
-      expect(logger).to receive(:debug).with(/Ignoring hostdev as it does not/)
-      expect(libvirt_client).to receive(:list_all_networks).and_return([default_network, hostdev_network, additional_network])
-
-      expect(subject.libvirt_networks(libvirt_client)).to match_array([
         hash_including(:name => 'default'),
         hash_including(:name => 'vagrant-libvirt'),
       ])