From 6d3cb071b4df976845db97401d3d3e013b09cbb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 20 Jul 2020 15:59:51 +0100 Subject: [PATCH] vmx: fix logic handling mac address type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the current formatter, the XML snippets: result in ethernet1.present = "true" ethernet1.networkName = "br1" ethernet1.connectionType = "bridged" ethernet1.addressType = "static" ethernet1.address = "00:0c:29:dd:ee:fe" ethernet1.checkMACAddress = "false" ethernet2.present = "true" ethernet2.networkName = "br2" ethernet2.connectionType = "bridged" ethernet2.addressType = "static" ethernet2.address = "aa:bb:cc:dd:ee:fd" ethernet2.checkMACAddress = "false" which is flawed, as both type='static' and type='generated' in the XML turn into 'static' in the VMX config. The existence of the 'static' attribute is further overriding whether the checkMACAddress config option is set as a side effect. Both these pieces of flawed logic were introduced in commit 454e5961abf40c14f8b6d7ee216229e68fd170bf Author: Bastien Orivel Date: Mon Jul 13 16:28:53 2020 +0200 Add a type attribute on the mac address element which intentionally added the 'checkMACAddress' side effect based on the 'type' attribute. With this change, we're reverting the handling of checkMACAddress to match what existed historically. The 'type' attribute now directly maps to the addressType attribute, so the above config becomes: ethernet1.present = "true" ethernet1.networkName = "br1" ethernet1.connectionType = "bridged" ethernet1.addressType = "static" ethernet1.address = "00:0c:29:dd:ee:fe" ethernet2.present = "true" ethernet2.networkName = "br2" ethernet2.connectionType = "bridged" ethernet2.addressType = "generated" ethernet2.generatedAddress = "aa:bb:cc:dd:ee:fd" ethernet2.generatedAddressOffset = "0" Reviewed-by: Michal Privoznik Signed-off-by: Daniel P. Berrangé --- src/vmx/vmx.c | 55 +++++++++++++------ .../xml2vmxdata/xml2vmx-ethernet-mac-type.vmx | 7 +-- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 97ec84446a..f0a45089cc 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -3732,7 +3732,9 @@ virVMXFormatEthernet(virDomainNetDefPtr def, int controller, virBufferPtr buffer, int virtualHW_version) { char mac_string[VIR_MAC_STRING_BUFLEN]; - const bool staticMac = def->mac_type == VIR_DOMAIN_NET_MAC_TYPE_STATIC; + virDomainNetMacType mac_type = VIR_DOMAIN_NET_MAC_TYPE_DEFAULT; + virTristateBool mac_check = VIR_TRISTATE_BOOL_ABSENT; + bool mac_vpx = false; unsigned int prefix, suffix; /* @@ -3830,31 +3832,48 @@ virVMXFormatEthernet(virDomainNetDefPtr def, int controller, prefix = (def->mac.addr[0] << 16) | (def->mac.addr[1] << 8) | def->mac.addr[2]; suffix = (def->mac.addr[3] << 16) | (def->mac.addr[4] << 8) | def->mac.addr[5]; - if (prefix == 0x000c29 && !staticMac) { - virBufferAsprintf(buffer, "ethernet%d.addressType = \"generated\"\n", - controller); - virBufferAsprintf(buffer, "ethernet%d.generatedAddress = \"%s\"\n", - controller, mac_string); - virBufferAsprintf(buffer, "ethernet%d.generatedAddressOffset = \"0\"\n", - controller); - } else if (prefix == 0x005056 && suffix <= 0x3fffff && !staticMac) { - virBufferAsprintf(buffer, "ethernet%d.addressType = \"static\"\n", - controller); - virBufferAsprintf(buffer, "ethernet%d.address = \"%s\"\n", - controller, mac_string); - } else if (prefix == 0x005056 && suffix >= 0x800000 && suffix <= 0xbfffff && !staticMac) { - virBufferAsprintf(buffer, "ethernet%d.addressType = \"vpx\"\n", - controller); + /* + * Historically we've not stored all the MAC related properties + * explicitly in the XML, so we must figure out some defaults + * based on the address ranges. + */ + if (prefix == 0x000c29) { + mac_type = VIR_DOMAIN_NET_MAC_TYPE_GENERATED; + } else if (prefix == 0x005056 && suffix <= 0x3fffff) { + mac_type = VIR_DOMAIN_NET_MAC_TYPE_STATIC; + } else if (prefix == 0x005056 && suffix >= 0x800000 && suffix <= 0xbfffff) { + mac_type = VIR_DOMAIN_NET_MAC_TYPE_GENERATED; + mac_vpx = true; + } else { + mac_type = VIR_DOMAIN_NET_MAC_TYPE_STATIC; + mac_check = VIR_TRISTATE_BOOL_NO; + } + + /* If explicit MAC type is set, ignore the above defaults */ + if (def->mac_type != VIR_DOMAIN_NET_MAC_TYPE_DEFAULT) { + mac_type = def->mac_type; + if (mac_type == VIR_DOMAIN_NET_MAC_TYPE_GENERATED) + mac_check = VIR_TRISTATE_BOOL_ABSENT; + } + + if (mac_type == VIR_DOMAIN_NET_MAC_TYPE_GENERATED) { + virBufferAsprintf(buffer, "ethernet%d.addressType = \"%s\"\n", + controller, mac_vpx ? "vpx" : "generated"); virBufferAsprintf(buffer, "ethernet%d.generatedAddress = \"%s\"\n", controller, mac_string); + if (!mac_vpx) + virBufferAsprintf(buffer, "ethernet%d.generatedAddressOffset = \"0\"\n", + controller); } else { virBufferAsprintf(buffer, "ethernet%d.addressType = \"static\"\n", controller); virBufferAsprintf(buffer, "ethernet%d.address = \"%s\"\n", controller, mac_string); - virBufferAsprintf(buffer, "ethernet%d.checkMACAddress = \"false\"\n", - controller); } + if (mac_check != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buffer, "ethernet%d.checkMACAddress = \"%s\"\n", + controller, + mac_check == VIR_TRISTATE_BOOL_YES ? "true" : "false"); return 0; } diff --git a/tests/xml2vmxdata/xml2vmx-ethernet-mac-type.vmx b/tests/xml2vmxdata/xml2vmx-ethernet-mac-type.vmx index 212b3f192c..061aed3010 100644 --- a/tests/xml2vmxdata/xml2vmx-ethernet-mac-type.vmx +++ b/tests/xml2vmxdata/xml2vmx-ethernet-mac-type.vmx @@ -20,10 +20,9 @@ ethernet1.networkName = "br1" ethernet1.connectionType = "bridged" ethernet1.addressType = "static" ethernet1.address = "00:0c:29:dd:ee:fe" -ethernet1.checkMACAddress = "false" ethernet2.present = "true" ethernet2.networkName = "br2" ethernet2.connectionType = "bridged" -ethernet2.addressType = "static" -ethernet2.address = "aa:bb:cc:dd:ee:fd" -ethernet2.checkMACAddress = "false" +ethernet2.addressType = "generated" +ethernet2.generatedAddress = "aa:bb:cc:dd:ee:fd" +ethernet2.generatedAddressOffset = "0"