[PATCH] Add a check attribute on the mac address element

Bastien Orivel posted 1 patch 3 years, 8 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200713094413.30201-1-bastien.orivel@diateam.net
NEWS.rst                                      |  6 ++++
docs/drvesx.html.in                           |  5 ++++
docs/schemas/domaincommon.rng                 |  3 ++
src/conf/domain_conf.c                        | 18 +++++++++++-
src/conf/domain_conf.h                        |  1 +
src/vmx/vmx.c                                 |  9 +++++-
.../network-interface-mac-check.xml           | 29 +++++++++++++++++++
tests/genericxml2xmltest.c                    |  2 ++
8 files changed, 71 insertions(+), 2 deletions(-)
create mode 100644 tests/genericxml2xmlindata/network-interface-mac-check.xml
[PATCH] Add a check attribute on the mac address element
Posted by Bastien Orivel 3 years, 8 months ago
This is only used in the ESX driver where, when set to "no", 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@diateam.net>
---
 NEWS.rst                                      |  6 ++++
 docs/drvesx.html.in                           |  5 ++++
 docs/schemas/domaincommon.rng                 |  3 ++
 src/conf/domain_conf.c                        | 18 +++++++++++-
 src/conf/domain_conf.h                        |  1 +
 src/vmx/vmx.c                                 |  9 +++++-
 .../network-interface-mac-check.xml           | 29 +++++++++++++++++++
 tests/genericxml2xmltest.c                    |  2 ++
 8 files changed, 71 insertions(+), 2 deletions(-)
 create mode 100644 tests/genericxml2xmlindata/network-interface-mac-check.xml

diff --git a/NEWS.rst b/NEWS.rst
index 1928220854..ac4de4360d 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 ``check`` attribute for mac addresses.
+
+    This attribute allows (when set to ``no``) ignoring VMWare checks of the
+    MAC addresses that would generate a new one if they were in its OUI
+    (00:0c:29).
+
 * **Improvements**
 
   * esx: Change the NIC limit for recent virtualHW versions
diff --git a/docs/drvesx.html.in b/docs/drvesx.html.in
index ac7bc645d1..90f368e4f5 100644
--- a/docs/drvesx.html.in
+++ b/docs/drvesx.html.in
@@ -427,6 +427,11 @@ error: invalid argument in libvirt was built without the 'esx' driver
 <pre>
 ethernet0.checkMACAddress = "false"
 </pre>
+    <p>
+        <span class="since">Since 6.6.0</span>, one can force libvirt to keep the
+        provided MAC address when it's in the reserved VMware range by adding a
+        <code>check="no"</code> attribute to the <code>&lt;mac/&gt;</code> element.
+    </p>
 
 
     <h3><a id="hardware">Available hardware</a></h3>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 4b4aa60c66..b926b752fe 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3179,6 +3179,9 @@
           <attribute name="address">
             <ref name="uniMacAddr"/>
           </attribute>
+          <optional>
+            <attribute name="check"><ref name="virYesNo"/></attribute>
+          </optional>
           <empty/>
         </element>
       </optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d14485f18d..aa1417c4d8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11904,6 +11904,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
     virDomainChrSourceReconnectDef reconnect = {0};
     int rv, val;
     g_autofree char *macaddr = NULL;
+    g_autofree char *macaddr_check = NULL;
     g_autofree char *type = NULL;
     g_autofree char *network = NULL;
     g_autofree char *portgroup = NULL;
@@ -11984,6 +11985,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
             }
             if (!macaddr && virXMLNodeNameEqual(cur, "mac")) {
                 macaddr = virXMLPropString(cur, "address");
+                macaddr_check = virXMLPropString(cur, "check");
             } else if (!network &&
                        def->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
                        virXMLNodeNameEqual(cur, "source")) {
@@ -12173,6 +12175,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
         def->mac_generated = true;
     }
 
+    if (macaddr_check) {
+        int tmpCheck;
+        if ((tmpCheck = virTristateBoolTypeFromString(macaddr_check)) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("invalid mac address check value: '%s'"),
+                           macaddr_check);
+            goto error;
+        }
+        def->mac_check = tmpCheck;
+    }
+
     if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info,
                                     flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT
                                     | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) {
@@ -26468,8 +26481,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_check != VIR_TRISTATE_BOOL_ABSENT)
+        virBufferAsprintf(buf, " check='%s'", virTristateBoolTypeToString(def->mac_check));
+    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..f2bcd62857 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -972,6 +972,7 @@ struct _virDomainNetDef {
     virDomainNetType type;
     virMacAddr mac;
     bool mac_generated; /* true if mac was *just now* auto-generated by libvirt */
+    virTristateBool mac_check;
     int model; /* virDomainNetModelType */
     char *modelstr;
     union {
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index d4d66f6768..82035884a2 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -3829,7 +3829,14 @@ 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) {
+    if (def->mac_check == VIR_TRISTATE_BOOL_NO) {
+        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);
+    } else if (prefix == 0x000c29) {
         virBufferAsprintf(buffer, "ethernet%d.addressType = \"generated\"\n",
                           controller);
         virBufferAsprintf(buffer, "ethernet%d.generatedAddress = \"%s\"\n",
diff --git a/tests/genericxml2xmlindata/network-interface-mac-check.xml b/tests/genericxml2xmlindata/network-interface-mac-check.xml
new file mode 100644
index 0000000000..b21577d7cc
--- /dev/null
+++ b/tests/genericxml2xmlindata/network-interface-mac-check.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' check='yes'/>
+      <source bridge='br1'/>
+    </interface>
+    <interface type='bridge'>
+      <mac address='aa:bb:cc:dd:ee:fd' check='no'/>
+      <source bridge='br2'/>
+    </interface>
+  </devices>
+</domain>
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 8b9b0bafb6..102abfdec2 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-check");
+
     DO_TEST_DIFFERENT("chardev-tcp");
     DO_TEST_FULL("chardev-tcp-missing-host", 0, false,
                  TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE);
-- 
2.20.1

Re: [PATCH] Add a check attribute on the mac address element
Posted by Michal Privoznik 3 years, 8 months ago
On 7/13/20 11:44 AM, Bastien Orivel wrote:
> This is only used in the ESX driver where, when set to "no", 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@diateam.net>
> ---
>   NEWS.rst                                      |  6 ++++
>   docs/drvesx.html.in                           |  5 ++++
>   docs/schemas/domaincommon.rng                 |  3 ++
>   src/conf/domain_conf.c                        | 18 +++++++++++-
>   src/conf/domain_conf.h                        |  1 +
>   src/vmx/vmx.c                                 |  9 +++++-
>   .../network-interface-mac-check.xml           | 29 +++++++++++++++++++
>   tests/genericxml2xmltest.c                    |  2 ++
>   8 files changed, 71 insertions(+), 2 deletions(-)
>   create mode 100644 tests/genericxml2xmlindata/network-interface-mac-check.xml
> 
> diff --git a/NEWS.rst b/NEWS.rst
> index 1928220854..ac4de4360d 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 ``check`` attribute for mac addresses.
> +
> +    This attribute allows (when set to ``no``) ignoring VMWare checks of the
> +    MAC addresses that would generate a new one if they were in its OUI
> +    (00:0c:29).
> +
>   * **Improvements**
>   


While you get bonus points for remembering to document this change, it 
should go into a separate patch, because keeping it in a single one 
usually leads to conflicts on backports.

But anyway, looking at virVMXFormatEthernet() - why don't we set all 
MACs 'static'? Alternatively, we can use @mac_generated member to 
determine whether the MAC address was provided by user or automagically 
generated (and use static/generated addressType accrodingly)?

I mean, this attribute you are adding (the code is correct though) 
should be last resort. Can you please elaborate more?

Michal

Re: [PATCH] Add a check attribute on the mac address element
Posted by Bastien Orivel 3 years, 8 months ago

On 13/07/2020 13:41, Michal Privoznik wrote:
> On 7/13/20 11:44 AM, Bastien Orivel wrote:
>> This is only used in the ESX driver where, when set to "no", 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.
>>
>
> While you get bonus points for remembering to document this change, it
> should go into a separate patch, because keeping it in a single one
> usually leads to conflicts on backports.
Oops, didn't know about that, will split in a v2 once we resolved the
other question.

> But anyway, looking at virVMXFormatEthernet() - why don't we set all
> MACs 'static'? Alternatively, we can use @mac_generated member to
> determine whether the MAC address was provided by user or
> automagically generated (and use static/generated addressType
> accrodingly)?

Mostly because I didn't want to break any existing script/setup that
relies on the fact that libvirt would set the address type to what it is
right now. For example, right now, providing a MAC address in the
@00:0c:29@ range makes the ESXi server generate a new MAC address on
define. I can imagine that some people outside always provide the same
MAC address to define their machine and rely on libvirt/ESXi to generate
a new one in that range for them.



Bastien


Re: [PATCH] Add a check attribute on the mac address element
Posted by Michal Privoznik 3 years, 8 months ago
On 7/13/20 1:50 PM, Bastien Orivel wrote:
> 
> 
> On 13/07/2020 13:41, Michal Privoznik wrote:
>> On 7/13/20 11:44 AM, Bastien Orivel wrote:
>>> This is only used in the ESX driver where, when set to "no", 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.
>>>
>>
>> While you get bonus points for remembering to document this change, it
>> should go into a separate patch, because keeping it in a single one
>> usually leads to conflicts on backports.
> Oops, didn't know about that, will split in a v2 once we resolved the
> other question.
> 
>> But anyway, looking at virVMXFormatEthernet() - why don't we set all
>> MACs 'static'? Alternatively, we can use @mac_generated member to
>> determine whether the MAC address was provided by user or
>> automagically generated (and use static/generated addressType
>> accrodingly)?
> 
> Mostly because I didn't want to break any existing script/setup that
> relies on the fact that libvirt would set the address type to what it is
> right now. For example, right now, providing a MAC address in the
> @00:0c:29@ range makes the ESXi server generate a new MAC address on
> define.

> I can imagine that some people outside always provide the same
> MAC address to define their machine and rely on libvirt/ESXi to generate
> a new one in that range for them.

This doesn't sound right. If they provide a MAC address we should 
guarantee it, if they don't provide address we should generate one, that 
suits hypervisor's needs.

Pino, you have more experience with ESX than I do, do you know whether 
we could do the switch to mark MAC addresses static/generated according 
to @mac_generated?

Michal

Re: [PATCH] Add a check attribute on the mac address element
Posted by Daniel P. Berrangé 3 years, 8 months ago
On Mon, Jul 13, 2020 at 02:08:59PM +0200, Michal Privoznik wrote:
> On 7/13/20 1:50 PM, Bastien Orivel wrote:
> > 
> > 
> > On 13/07/2020 13:41, Michal Privoznik wrote:
> > > On 7/13/20 11:44 AM, Bastien Orivel wrote:
> > > > This is only used in the ESX driver where, when set to "no", 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.
> > > > 
> > > 
> > > While you get bonus points for remembering to document this change, it
> > > should go into a separate patch, because keeping it in a single one
> > > usually leads to conflicts on backports.
> > Oops, didn't know about that, will split in a v2 once we resolved the
> > other question.
> > 
> > > But anyway, looking at virVMXFormatEthernet() - why don't we set all
> > > MACs 'static'? Alternatively, we can use @mac_generated member to
> > > determine whether the MAC address was provided by user or
> > > automagically generated (and use static/generated addressType
> > > accrodingly)?
> > 
> > Mostly because I didn't want to break any existing script/setup that
> > relies on the fact that libvirt would set the address type to what it is
> > right now. For example, right now, providing a MAC address in the
> > @00:0c:29@ range makes the ESXi server generate a new MAC address on
> > define.
> 
> > I can imagine that some people outside always provide the same
> > MAC address to define their machine and rely on libvirt/ESXi to generate
> > a new one in that range for them.
> 
> This doesn't sound right. If they provide a MAC address we should guarantee
> it, if they don't provide address we should generate one, that suits
> hypervisor's needs.
> 
> Pino, you have more experience with ESX than I do, do you know whether we
> could do the switch to mark MAC addresses static/generated according to
> @mac_generated?

That's an internal thing though - if we want to support that properly for
ESX, then I think we need to make that an explicit type="static|generated"
attribute. In ESX there are different ranges you need to use for static vs
generated tooo.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] Add a check attribute on the mac address element
Posted by Michal Privoznik 3 years, 8 months ago
On 7/13/20 2:15 PM, Daniel P. Berrangé wrote:
> On Mon, Jul 13, 2020 at 02:08:59PM +0200, Michal Privoznik wrote:

>>
>> Pino, you have more experience with ESX than I do, do you know whether we
>> could do the switch to mark MAC addresses static/generated according to
>> @mac_generated?
> 
> That's an internal thing though - if we want to support that properly for
> ESX, then I think we need to make that an explicit type="static|generated"
> attribute. In ESX there are different ranges you need to use for static vs
> generated tooo.

Indeed there are. So far virVMXFormatEthernet() puts static/generated 
into the config based on these ranges.
Seems like there is simple way out and we need to have the argument. 
Bastien, can you please post v2 where the attribute is renamed to @type 
and accepts static/generated values?

Michal

Re: [PATCH] Add a check attribute on the mac address element
Posted by Daniel P. Berrangé 3 years, 8 months ago
On Mon, Jul 13, 2020 at 11:44:13AM +0200, Bastien Orivel wrote:
> This is only used in the ESX driver where, when set to "no", 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@diateam.net>
> ---


> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index d4d66f6768..82035884a2 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -3829,7 +3829,14 @@ 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) {
> +    if (def->mac_check == VIR_TRISTATE_BOOL_NO) {
> +        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);
> +    } else if (prefix == 0x000c29) {
>          virBufferAsprintf(buffer, "ethernet%d.addressType = \"generated\"\n",
>                            controller);
>          virBufferAsprintf(buffer, "ethernet%d.generatedAddress = \"%s\"\n",

I don't think it is correct to merely add another branch to the if
at the start.  The new "check" attribute should apply to all of
the existing branches.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|