[PATCHv2 1/2] Add a type attribute on the mac address element
Michal Privoznik
mprivozn at redhat.com
Mon Jul 13 16:53:52 UTC 2020
On 7/13/20 4:28 PM, Bastien Orivel wrote:
> This is only used in the ESX driver where, when set to "static", it will
> ignore all the checks libvirt does about the origin of the MAC address
> (whether or not it's in a VMWare OUI) and forward the original one to
> the ESX server telling it not to check it either.
>
> This allows keeping a deterministic MAC address which can be useful for
> licensed software which might dislike changes.
>
> Signed-off-by: Bastien Orivel <bastien.orivel at diateam.net>
> ---
> NEWS.rst | 6 ++++
> docs/schemas/domaincommon.rng | 8 +++++
> src/conf/domain_conf.c | 26 ++++++++++++++++-
> src/conf/domain_conf.h | 11 +++++++
> src/vmx/vmx.c | 8 +++--
> .../network-interface-mac-type.xml | 29 +++++++++++++++++++
> tests/genericxml2xmltest.c | 2 ++
> 7 files changed, 86 insertions(+), 4 deletions(-)
> create mode 100644 tests/genericxml2xmlindata/network-interface-mac-type.xml
>
> diff --git a/NEWS.rst b/NEWS.rst
> index 1928220854..2c6c628c61 100644
> --- a/NEWS.rst
> +++ b/NEWS.rst
> @@ -18,6 +18,12 @@ v6.6.0 (unreleased)
> Libvirt allows configuring ACPI Heterogeneous Memory Attribute Table to
> hint software running inside the guest on optimization.
>
> + * esx: Add a ``type`` attribute for mac addresses.
> +
> + This attribute allows (when set to ``static``) ignoring VMWare checks of the
> + MAC addresses that would generate a new one if they were in its OUI
> + (00:0c:29).
> +
> * **Improvements**
Now that I'm re-reading my comment to v1 I realize that I might have
misled you. What I meant was this NEWS.rst change must go into a
separate patch. The reason is, while this change happens in 6.6.0 if
cherry-picked to say 6.5.0 the corresponding NEWS.rst section doesn't
exist and hence it wouldn't be a clean cherry-pick.
The documentation of new XML attribute can of course go with the code
that's introducing it (in fact, it should).
>
> * esx: Change the NIC limit for recent virtualHW versions
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 4b4aa60c66..a810f569c6 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3179,6 +3179,14 @@
> <attribute name="address">
> <ref name="uniMacAddr"/>
> </attribute>
> + <optional>
> + <attribute name="type">
> + <choice>
> + <value>generated</value>
> + <value>static</value>
> + </choice>
> + </attribute>
> + </optional>
> <empty/>
> </element>
> </optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d14485f18d..93e203de23 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -611,6 +611,13 @@ VIR_ENUM_IMPL(virDomainChrDeviceState,
> "disconnected",
> );
>
> +VIR_ENUM_IMPL(virDomainNetMacType,
> + VIR_DOMAIN_NET_MAC_TYPE_LAST,
> + "",
> + "generated",
> + "static",
> +);
> +
> VIR_ENUM_IMPL(virDomainChrSerialTarget,
> VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST,
> "none",
> @@ -11904,6 +11911,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> virDomainChrSourceReconnectDef reconnect = {0};
> int rv, val;
> g_autofree char *macaddr = NULL;
> + g_autofree char *macaddr_type = NULL;
> g_autofree char *type = NULL;
> g_autofree char *network = NULL;
> g_autofree char *portgroup = NULL;
> @@ -11984,6 +11992,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> if (!macaddr && virXMLNodeNameEqual(cur, "mac")) {
> macaddr = virXMLPropString(cur, "address");
> + macaddr_type = virXMLPropString(cur, "type");
> } else if (!network &&
> def->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> virXMLNodeNameEqual(cur, "source")) {
> @@ -12173,6 +12182,18 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> def->mac_generated = true;
> }
>
> + if (macaddr_type) {
> + virDomainNetMacType tmp;
This should be type of int. The reason is that upon error
TypeFromString() will return a negative value and we are not guaranteed
that typecast to the enum keeps the value negative.
> + if ((tmp = virDomainNetMacTypeTypeFromString(macaddr_type)) <= 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("invalid mac address check value: '%s'. Valid "
> + "values are \"generated\" and \"static\"."),
> + macaddr_type);
> + goto error;
> + }
> + def->mac_type = tmp;
> + }
> +
> if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info,
> flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT
> | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) {
> @@ -26468,8 +26489,11 @@ virDomainNetDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, ">\n");
>
> virBufferAdjustIndent(buf, 2);
> - virBufferAsprintf(buf, "<mac address='%s'/>\n",
> + virBufferAsprintf(buf, "<mac address='%s'",
> virMacAddrFormat(&def->mac, macstr));
> + if (def->mac_type)
> + virBufferAsprintf(buf, " type='%s'", virDomainNetMacTypeTypeToString(def->mac_type));
> + virBufferAddLit(buf, "/>\n");
>
> if (publicActual) {
> /* when there is a virDomainActualNetDef, and we haven't been
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 6a737591e2..7b754f959d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -921,6 +921,15 @@ typedef enum {
> VIR_DOMAIN_NET_VIRTIO_TX_MODE_LAST
> } virDomainNetVirtioTxModeType;
>
> +/* whether a mac address should be marked as generated in the esx driver or not*/
> +typedef enum {
> + VIR_DOMAIN_NET_MAC_TYPE_DEFAULT, /* generated */
As mentioned on the IRC, this should be:
VIR_DOMAIN_NET_MAC_TYPE_DEFAULT = 0
to make it explicit that value zero means default (and that when the
corresponding virDomainNetDef structure is allocated the _DEFAULT is
'put in').
> + VIR_DOMAIN_NET_MAC_TYPE_GENERATED,
> + VIR_DOMAIN_NET_MAC_TYPE_STATIC,
> +
> + VIR_DOMAIN_NET_MAC_TYPE_LAST
> +} virDomainNetMacType;
> +
> /* the type of teaming device */
> typedef enum {
> VIR_DOMAIN_NET_TEAMING_TYPE_NONE,
> @@ -972,6 +981,7 @@ struct _virDomainNetDef {
> virDomainNetType type;
> virMacAddr mac;
> bool mac_generated; /* true if mac was *just now* auto-generated by libvirt */
> + virDomainNetMacType mac_type;
> int model; /* virDomainNetModelType */
> char *modelstr;
> union {
> @@ -3556,6 +3566,7 @@ VIR_ENUM_DECL(virDomainFSCacheMode);
> VIR_ENUM_DECL(virDomainNet);
> VIR_ENUM_DECL(virDomainNetBackend);
> VIR_ENUM_DECL(virDomainNetVirtioTxMode);
> +VIR_ENUM_DECL(virDomainNetMacType);
> VIR_ENUM_DECL(virDomainNetTeaming);
> VIR_ENUM_DECL(virDomainNetInterfaceLinkState);
> VIR_ENUM_DECL(virDomainNetModel);
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index d4d66f6768..b307072869 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -3829,19 +3829,21 @@ 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) {
> + bool staticMac = def->mac_type == VIR_DOMAIN_NET_MAC_TYPE_STATIC;
We don't really like variables declared in the middle of a block.
> +
> + 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) {
> + } 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) {
> + } else if (prefix == 0x005056 && suffix >= 0x800000 && suffix <= 0xbfffff && !staticMac) {
> virBufferAsprintf(buffer, "ethernet%d.addressType = \"vpx\"\n",
> controller);
> virBufferAsprintf(buffer, "ethernet%d.generatedAddress = \"%s\"\n",
> diff --git a/tests/genericxml2xmlindata/network-interface-mac-type.xml b/tests/genericxml2xmlindata/network-interface-mac-type.xml
> new file mode 100644
> index 0000000000..2b690629bb
> --- /dev/null
> +++ b/tests/genericxml2xmlindata/network-interface-mac-type.xml
> @@ -0,0 +1,29 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <interface type='bridge'>
> + <mac address='aa:bb:cc:dd:ee:ff'/>
> + <source bridge='br0'/>
> + </interface>
> + <interface type='bridge'>
> + <mac address='aa:bb:cc:dd:ee:fe' type='static'/>
> + <source bridge='br1'/>
> + </interface>
> + <interface type='bridge'>
> + <mac address='aa:bb:cc:dd:ee:fd' type='generated'/>
> + <source bridge='br2'/>
> + </interface>
> + </devices>
> +</domain>
> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
> index 8b9b0bafb6..ea6585c901 100644
> --- a/tests/genericxml2xmltest.c
> +++ b/tests/genericxml2xmltest.c
> @@ -183,6 +183,8 @@ mymain(void)
> DO_TEST("cpu-cache-passthrough");
> DO_TEST("cpu-cache-disable");
>
> + DO_TEST("network-interface-mac-type");
> +
> DO_TEST_DIFFERENT("chardev-tcp");
> DO_TEST_FULL("chardev-tcp-missing-host", 0, false,
> TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
>
Just realized that we can move this to xml2vmxtest.c and xml2vmxdata.
That enables us to demonstrate that if a MAC address begins with the
right prefix (00:0c:29) but has type='static' the expected config is
generated.
Michal
More information about the libvir-list
mailing list