From 7a8306745b81019d85c0abe9814b534466a6bef6 Mon Sep 17 00:00:00 2001
From: Darragh Bailey <daragh.bailey@gmail.com>
Date: Fri, 9 Dec 2022 07:48:08 +0000
Subject: [PATCH] Handle autoport when port explicit set (#1693)

Better handle setting the autoport value when the port is explicitly set
to ensure that the XML sent to update the VM is correct and will be the
XML that is reflected in the defined machine.

By prioritizing checking if the port is provided, graphics_autoport =
"yes" is ignored.

Fixes: #1687
---
 lib/vagrant-libvirt/action/start_domain.rb | 22 +++++++++---
 spec/unit/action/start_domain_spec.rb      | 42 ++++++++++++++++++++++
 2 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/lib/vagrant-libvirt/action/start_domain.rb b/lib/vagrant-libvirt/action/start_domain.rb
index 9956834..66d7997 100644
--- a/lib/vagrant-libvirt/action/start_domain.rb
+++ b/lib/vagrant-libvirt/action/start_domain.rb
@@ -272,13 +272,25 @@ module VagrantPlugins
               graphics.attributes['listen'] = config.graphics_ip
               graphics.delete_element('//listen')
             end
-            if graphics.attributes['autoport'] != config.graphics_autoport
-              descr_changed = true
-              graphics.attributes['autoport'] = config.graphics_autoport
-              if config.graphics_autoport == 'no'
-                graphics.attributes.delete('autoport')
+            unless config.graphics_port.nil? or config.graphics_port == -1
+              if graphics.attributes['autoport'] != 'no'
+                descr_changed = true
+                graphics.attributes['autoport'] = 'no'
+              end
+              if graphics.attributes['port'] != config.graphics_port
+                descr_changed = true
                 graphics.attributes['port'] = config.graphics_port
               end
+            else
+              if graphics.attributes['autoport'] != config.graphics_autoport
+                descr_changed = true
+                graphics.attributes['autoport'] = config.graphics_autoport
+                if config.graphics_autoport == 'no'
+                  graphics.attributes['port'] = config.graphics_port
+                else
+                  graphics.attributes['port'] = '-1'
+                end
+              end
             end
             if graphics.attributes['websocket'] != config.graphics_websocket.to_s
               descr_changed = true
diff --git a/spec/unit/action/start_domain_spec.rb b/spec/unit/action/start_domain_spec.rb
index 07927ad..a8826ef 100644
--- a/spec/unit/action/start_domain_spec.rb
+++ b/spec/unit/action/start_domain_spec.rb
@@ -349,6 +349,48 @@ describe VagrantPlugins::ProviderLibvirt::Action::StartDomain do
           expect(subject.call(env)).to be_nil
         end
       end
+
+      [
+        [
+          'when port explicitly set, should set autoport=no',
+          proc { |config|
+            config.graphics_port = 5901
+          },
+          "<graphics autoport='yes' keymap='en-us' listen='127.0.0.1' port='-1' type='vnc' websocket='-1'/>",
+          "<graphics autoport='no' keymap='en-us' listen='127.0.0.1' port='5901' type='vnc' websocket='-1'/>",
+        ],
+        [
+          'when port updated, should set autoport=no and update port',
+          proc { |config|
+            config.graphics_port = 5902
+          },
+          "<graphics autoport='no' keymap='en-us' listen='127.0.0.1' port='5901' type='vnc' websocket='-1'/>",
+          "<graphics autoport='no' keymap='en-us' listen='127.0.0.1' port='5902' type='vnc' websocket='-1'/>",
+        ],
+        [
+          'when autoport set and no port, should set autoport=yes and update port to -1',
+          proc { |config|
+            config.graphics_autoport = 'yes'
+          },
+          "<graphics autoport='no' keymap='en-us' listen='127.0.0.1' port='5901' type='vnc' websocket='-1'/>",
+          "<graphics autoport='yes' keymap='en-us' listen='127.0.0.1' port='-1' type='vnc' websocket='-1'/>",
+        ],
+      ].each do |description, config_proc, graphics_xml_start, graphics_xml_output|
+        it "#{description}" do
+          config_proc.call(machine.provider_config)
+
+          initial_domain_xml = domain_xml.gsub(/<graphics .*\/>/, graphics_xml_start)
+          updated_domain_xml = domain_xml.gsub(/<graphics .*\/>/, graphics_xml_output)
+
+          expect(ui).to_not receive(:warn)
+          expect(connection).to receive(:define_domain).with(match(graphics_xml_output)).and_return(libvirt_domain)
+          expect(libvirt_domain).to receive(:xml_desc).and_return(initial_domain_xml, updated_domain_xml, updated_domain_xml)
+          expect(libvirt_domain).to receive(:autostart=)
+          expect(domain).to receive(:start)
+
+          expect(subject.call(env)).to be_nil
+        end
+      end
     end
 
     context 'nvram' do