Select better defaults when graphics type is spice (#1625)

Reduce the number of other graphics settings that need to be adjusted
once the type has been set to spice by defaulting the remaining options
to ones better suited for spice, in addition to adding the required
channel automatically.

Fixes: #1482

lib/vagrant-libvirt/config.rb

@@ -901,12 +901,12 @@ module VagrantPlugins
            @graphics_passwd == UNSET_VALUE
           @graphics_passwd = nil
         end
-        @graphics_port = -1 if @graphics_port == UNSET_VALUE
+        @graphics_port = @graphics_type == 'spice' ? nil : -1 if @graphics_port == UNSET_VALUE
-        @graphics_ip = '127.0.0.1' if @graphics_ip == UNSET_VALUE
+        @graphics_ip = @graphics_type == 'spice' ? nil : '127.0.0.1' if @graphics_ip == UNSET_VALUE
-        @video_type = 'cirrus' if @video_type == UNSET_VALUE
-        @video_vram = 16384 if @video_vram == UNSET_VALUE
         @video_accel3d = false if @video_accel3d == UNSET_VALUE
         @graphics_gl = @video_accel3d if @graphics_gl == UNSET_VALUE
+        @video_type = @video_accel3d ? 'virtio' : 'cirrus' if @video_type == UNSET_VALUE
+        @video_vram = 16384 if @video_vram == UNSET_VALUE
         @sound_type = nil if @sound_type == UNSET_VALUE
         @keymap = 'en-us' if @keymap == UNSET_VALUE
         @kvm_hidden = false if @kvm_hidden == UNSET_VALUE
@@ -938,12 +938,15 @@ module VagrantPlugins
         @inputs = [{ type: 'mouse', bus: 'ps2' }] if @inputs == UNSET_VALUE
 
         # Channels
-        if @channels == UNSET_VALUE
+        @channels = [] if @channels == UNSET_VALUE
-          @channels = []
+        if @qemu_use_agent == true
-          if @qemu_use_agent == true
+          if @channels.all? { |channel| !channel.fetch(:target_name, '').start_with?('org.qemu.guest_agent.') }
-            if @channels.all? { |channel| !channel.fetch(:target_name, '').start_with?('org.qemu.guest_agent.') }
+            channel(:type => 'unix', :target_name => 'org.qemu.guest_agent.0', :target_type => 'virtio')
-              channel(:type => 'unix', :target_name => 'org.qemu.guest_agent.0', :target_type => 'virtio')
+          end
-            end
+        end
+        if @graphics_type == 'spice'
+          if @channels.all? { |channel| !channel.fetch(:target_name, '').start_with?('com.redhat.spice.') }
+            channel(:type => 'spicevmc', :target_name => 'com.redhat.spice.0', :target_type => 'virtio')
           end
         end
 

spec/unit/config_spec.rb

@@ -559,6 +559,19 @@ describe VagrantPlugins::ProviderLibvirt::Config do
           expect(subject.channels).to match([a_hash_including({:target_name => 'org.qemu.guest_agent.0'})])
         end
 
+        context 'another channel type already defined' do
+          it 'should inject a qemu agent channel' do
+            subject.channel :type => 'spicevmc', :target_name => 'com.redhat.spice.0', :target_type => 'virtio'
+            subject.finalize!
+
+            expect(subject.channels).to_not be_empty
+            expect(subject.channels).to match([
+              a_hash_including({:target_name => 'com.redhat.spice.0'}),
+              a_hash_including({:target_name => 'org.qemu.guest_agent.0'}),
+            ])
+          end
+        end
+
         context 'when agent channel already added' do
           it 'should not modify the channels' do
             subject.channel :type => 'unix', :target_name => 'org.qemu.guest_agent.0', :target_type => 'virtio'
@@ -568,7 +581,7 @@ describe VagrantPlugins::ProviderLibvirt::Config do
             expect(subject.channels.length).to eq(1)
           end
 
-          context 'when agent channel explicitly disbaled' do
+          context 'when agent channel explicitly disabled' do
             it 'should not include an agent channel' do
               subject.channel :type => 'unix', :target_name => 'org.qemu.guest_agent.0', :disabled => true
 
@@ -579,6 +592,52 @@ describe VagrantPlugins::ProviderLibvirt::Config do
           end
         end
       end
+
+      context 'when graphics type set to spice' do
+        before do
+          subject.graphics_type = 'spice'
+        end
+
+        it 'should inject a spice agent channel' do
+          subject.finalize!
+
+          expect(subject.channels).to_not be_empty
+          expect(subject.channels).to match([a_hash_including({:target_name => 'com.redhat.spice.0'})])
+        end
+
+        context 'another channel type already defined' do
+          it 'should inject a spice agent channel' do
+            subject.channel :type => 'unix', :target_name => 'org.qemu.guest_agent.0', :target_type => 'virtio'
+            subject.finalize!
+
+            expect(subject.channels).to_not be_empty
+            expect(subject.channels).to match([
+              a_hash_including({:target_name => 'org.qemu.guest_agent.0'}),
+              a_hash_including({:target_name => 'com.redhat.spice.0'}),
+            ])
+          end
+        end
+
+        context 'when spice channel already added' do
+          it 'should not modify the channels' do
+            subject.channel :type => 'spicevmc', :target_name => 'com.redhat.spice.0', :target_type => 'virtio'
+
+            subject.finalize!
+
+            expect(subject.channels.length).to eq(1)
+          end
+
+          context 'when agent channel explicitly disabled' do
+            it 'should not include an agent channel' do
+              subject.channel :type => 'spicevmc', :target_name => 'com.redhat.spice.0', :disabled => true
+
+              subject.finalize!
+
+              expect(subject.channels).to be_empty
+            end
+          end
+        end
+      end
     end
 
     context '@inputs' do
@@ -596,6 +655,28 @@ describe VagrantPlugins::ProviderLibvirt::Config do
         expect(subject.inputs).to eq([{:bus=>"usb", :type=>"keyboard"}])
       end
     end
+
+    context '@graphics_* and @video_*' do
+      it 'should set reasonable defaults' do
+        subject.finalize!
+
+        expect(subject.graphics_type).to eq('vnc')
+        expect(subject.graphics_port).to eq(-1)
+        expect(subject.graphics_ip).to eq('127.0.0.1')
+        expect(subject.graphics_autoport).to eq('yes')
+        expect(subject.channels).to be_empty
+      end
+
+      it 'should handle graphics_type set to spice' do
+        subject.graphics_type = 'spice'
+        subject.finalize!
+
+        expect(subject.graphics_port).to eq(nil)
+        expect(subject.graphics_ip).to eq(nil)
+        expect(subject.graphics_autoport).to eq('yes')
+        expect(subject.channels).to match([a_hash_including({:target_name => 'com.redhat.spice.0'})])
+      end
+    end
   end
 
   def assert_invalid

spec/unit/templates/domain_all_settings.xml

@@ -107,7 +107,7 @@
       <gl enable='yes'/>
     </graphics>
     <video>
-      <model type='cirrus' vram='16384' heads='1'>
+      <model type='virtio' vram='16384' heads='1'>
         <acceleration accel3d='yes'/>
       </model>
     </video>