[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