From 624df5d8edefb4e0f352c416037cebae24a45b5a Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Sun, 2 Oct 2022 21:43:37 +0100 Subject: [PATCH] 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 | 23 +++--- spec/unit/config_spec.rb | 83 ++++++++++++++++++++- spec/unit/templates/domain_all_settings.xml | 2 +- 3 files changed, 96 insertions(+), 12 deletions(-) diff --git a/lib/vagrant-libvirt/config.rb b/lib/vagrant-libvirt/config.rb index 16f057f..55c4c91 100644 --- a/lib/vagrant-libvirt/config.rb +++ b/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_ip = '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 + @graphics_port = @graphics_type == 'spice' ? nil : -1 if @graphics_port == UNSET_VALUE + @graphics_ip = @graphics_type == 'spice' ? nil : '127.0.0.1' if @graphics_ip == 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 @qemu_use_agent == true - 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') - end + @channels = [] if @channels == UNSET_VALUE + if @qemu_use_agent == true + 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') + 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 diff --git a/spec/unit/config_spec.rb b/spec/unit/config_spec.rb index e3207a5..e0f0843 100644 --- a/spec/unit/config_spec.rb +++ b/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 diff --git a/spec/unit/templates/domain_all_settings.xml b/spec/unit/templates/domain_all_settings.xml index f3c058e..5bd7aa3 100644 --- a/spec/unit/templates/domain_all_settings.xml +++ b/spec/unit/templates/domain_all_settings.xml @@ -107,7 +107,7 @@