1. Patchew
  2. Libvirt
  3. View series

[PATCH] conf: Generate MAC address instead of keeping all zeroes

Martin Kletzander posted 1 patch 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/01546933f264d901d624005f8eb10a62f3de5333.1693581135.git.mkletzan@redhat.com
src/conf/domain_conf.c                        |  2 +-
src/util/virmacaddr.c                         |  5 +++++
src/util/virmacaddr.h                         |  1 +
.../network-interface-mac-clear.xml           | 21 +++++++++++++++++++
.../network-interface-mac-clear.xml           | 21 +++++++++++++++++++
tests/genericxml2xmltest.c                    |  4 +++-
6 files changed, 52 insertions(+), 2 deletions(-)
create mode 100644 tests/genericxml2xmlindata/network-interface-mac-clear.xml
create mode 100644 tests/genericxml2xmloutdata/network-interface-mac-clear.xml
[PATCH] conf: Generate MAC address instead of keeping all zeroes
Posted by Martin Kletzander 8 months ago 
When we parse <mac address="00:00:00:00:00:00"/> we keep that in memory
and pass it down to the hypervisor.  However, that MAC address is not
strictly valid as it is not marked as locally administered (bit 0x02)
but it is not even globally unique.  It is also used for loopback device
on Linux, for example.  And QEMU sees such MAC address just as "not
specified" and generates a new one that libvirt does not even know
about.  So to make the overall experience better we now generate it if
the supplied one is all clear.

Resolves: https://issues.redhat.com/browse/RHEL-974

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/conf/domain_conf.c                        |  2 +-
 src/util/virmacaddr.c                         |  5 +++++
 src/util/virmacaddr.h                         |  1 +
 .../network-interface-mac-clear.xml           | 21 +++++++++++++++++++
 .../network-interface-mac-clear.xml           | 21 +++++++++++++++++++
 tests/genericxml2xmltest.c                    |  4 +++-
 6 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 tests/genericxml2xmlindata/network-interface-mac-clear.xml
 create mode 100644 tests/genericxml2xmloutdata/network-interface-mac-clear.xml

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bb4f1fdb948d..652bd09b21b8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9675,7 +9675,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
             return NULL;
     }
 
-    if (!macaddr) {
+    if (!macaddr || virMacAddrIsAllClear(&def->mac)) {
         virDomainNetGenerateMAC(xmlopt, &def->mac);
         def->mac_generated = true;
     }
diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
index 073f298b5b66..e06bb200fc68 100644
--- a/src/util/virmacaddr.c
+++ b/src/util/virmacaddr.c
@@ -246,6 +246,11 @@ virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN])
     return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
 }
 
+bool virMacAddrIsAllClear(const virMacAddr *addr)
+{
+    return !virMacAddrCmpRaw(addr, (const unsigned char[VIR_MAC_BUFLEN]){0});
+}
+
 void
 virMacAddrFree(virMacAddr *addr)
 {
diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
index f32b58805a61..7b9eb7443bd1 100644
--- a/src/util/virmacaddr.h
+++ b/src/util/virmacaddr.h
@@ -58,6 +58,7 @@ int virMacAddrParseHex(const char* str,
 bool virMacAddrIsUnicast(const virMacAddr *addr);
 bool virMacAddrIsMulticast(const virMacAddr *addr);
 bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
+bool virMacAddrIsAllClear(const virMacAddr *addr);
 void virMacAddrFree(virMacAddr *addr);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMacAddr, virMacAddrFree);
diff --git a/tests/genericxml2xmlindata/network-interface-mac-clear.xml b/tests/genericxml2xmlindata/network-interface-mac-clear.xml
new file mode 100644
index 000000000000..41beda8a79bb
--- /dev/null
+++ b/tests/genericxml2xmlindata/network-interface-mac-clear.xml
@@ -0,0 +1,21 @@
+<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='x86_64' 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='00:00:00:00:00:00'/>
+      <source bridge='br0'/>
+    </interface>
+  </devices>
+</domain>
diff --git a/tests/genericxml2xmloutdata/network-interface-mac-clear.xml b/tests/genericxml2xmloutdata/network-interface-mac-clear.xml
new file mode 100644
index 000000000000..a7935fa9f4de
--- /dev/null
+++ b/tests/genericxml2xmloutdata/network-interface-mac-clear.xml
@@ -0,0 +1,21 @@
+<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='x86_64' 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='52:54:00:00:00:00'/>
+      <source bridge='br0'/>
+    </interface>
+  </devices>
+</domain>
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 3501eadf5597..bf160b7e0bef 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -187,6 +187,7 @@ mymain(void)
     DO_TEST("cpu-cache-disable");
 
     DO_TEST("network-interface-mac-check");
+    DO_TEST_DIFFERENT("network-interface-mac-clear");
 
     DO_TEST_DIFFERENT("chardev-tcp");
     DO_TEST_FAIL_ACTIVE("chardev-tcp-missing-host");
@@ -255,4 +256,5 @@ mymain(void)
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-VIR_TEST_MAIN(mymain)
+VIR_TEST_MAIN_PRELOAD(mymain,
+                      VIR_TEST_MOCK("virrandom"))
-- 
2.42.0
Re: [PATCH] conf: Generate MAC address instead of keeping all zeroes
Posted by Michal Prívozník 8 months ago 
On 9/1/23 17:12, Martin Kletzander wrote:
> When we parse <mac address="00:00:00:00:00:00"/> we keep that in memory
> and pass it down to the hypervisor.  However, that MAC address is not
> strictly valid as it is not marked as locally administered (bit 0x02)
> but it is not even globally unique.  It is also used for loopback device
> on Linux, for example.  And QEMU sees such MAC address just as "not
> specified" and generates a new one that libvirt does not even know
> about.  So to make the overall experience better we now generate it if
> the supplied one is all clear.

Please consider s/  / /g

> 
> Resolves: https://issues.redhat.com/browse/RHEL-974
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/conf/domain_conf.c                        |  2 +-
>  src/util/virmacaddr.c                         |  5 +++++
>  src/util/virmacaddr.h                         |  1 +
>  .../network-interface-mac-clear.xml           | 21 +++++++++++++++++++
>  .../network-interface-mac-clear.xml           | 21 +++++++++++++++++++
>  tests/genericxml2xmltest.c                    |  4 +++-
>  6 files changed, 52 insertions(+), 2 deletions(-)
>  create mode 100644 tests/genericxml2xmlindata/network-interface-mac-clear.xml
>  create mode 100644 tests/genericxml2xmloutdata/network-interface-mac-clear.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index bb4f1fdb948d..652bd09b21b8 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9675,7 +9675,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
>              return NULL;
>      }
>  
> -    if (!macaddr) {
> +    if (!macaddr || virMacAddrIsAllClear(&def->mac)) {
>          virDomainNetGenerateMAC(xmlopt, &def->mac);
>          def->mac_generated = true;
>      }
> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index 073f298b5b66..e06bb200fc68 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -246,6 +246,11 @@ virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN])
>      return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
>  }
>  
> +bool virMacAddrIsAllClear(const virMacAddr *addr)
> +{
> +    return !virMacAddrCmpRaw(addr, (const unsigned char[VIR_MAC_BUFLEN]){0});
> +}
> +
>  void
>  virMacAddrFree(virMacAddr *addr)
>  {
> diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
> index f32b58805a61..7b9eb7443bd1 100644
> --- a/src/util/virmacaddr.h
> +++ b/src/util/virmacaddr.h
> @@ -58,6 +58,7 @@ int virMacAddrParseHex(const char* str,
>  bool virMacAddrIsUnicast(const virMacAddr *addr);
>  bool virMacAddrIsMulticast(const virMacAddr *addr);
>  bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
> +bool virMacAddrIsAllClear(const virMacAddr *addr);
>  void virMacAddrFree(virMacAddr *addr);
>  
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMacAddr, virMacAddrFree);

Please expose the symbol in src/libvirt_private.syms too.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
Re: [PATCH] conf: Generate MAC address instead of keeping all zeroes
Posted by Martin Kletzander 8 months ago 
On Mon, Sep 04, 2023 at 02:34:49PM +0200, Michal Prívozník wrote:
>On 9/1/23 17:12, Martin Kletzander wrote:
>> When we parse <mac address="00:00:00:00:00:00"/> we keep that in memory
>> and pass it down to the hypervisor.  However, that MAC address is not
>> strictly valid as it is not marked as locally administered (bit 0x02)
>> but it is not even globally unique.  It is also used for loopback device
>> on Linux, for example.  And QEMU sees such MAC address just as "not
>> specified" and generates a new one that libvirt does not even know
>> about.  So to make the overall experience better we now generate it if
>> the supplied one is all clear.
>
>Please consider s/  / /g
>

OK, I'll do it for you this time ;)  But don't expect me to unlearn this
old monospace habit any time soon.

>>
>> Resolves: https://issues.redhat.com/browse/RHEL-974
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/conf/domain_conf.c                        |  2 +-
>>  src/util/virmacaddr.c                         |  5 +++++
>>  src/util/virmacaddr.h                         |  1 +
>>  .../network-interface-mac-clear.xml           | 21 +++++++++++++++++++
>>  .../network-interface-mac-clear.xml           | 21 +++++++++++++++++++
>>  tests/genericxml2xmltest.c                    |  4 +++-
>>  6 files changed, 52 insertions(+), 2 deletions(-)
>>  create mode 100644 tests/genericxml2xmlindata/network-interface-mac-clear.xml
>>  create mode 100644 tests/genericxml2xmloutdata/network-interface-mac-clear.xml
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index bb4f1fdb948d..652bd09b21b8 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -9675,7 +9675,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
>>              return NULL;
>>      }
>>
>> -    if (!macaddr) {
>> +    if (!macaddr || virMacAddrIsAllClear(&def->mac)) {
>>          virDomainNetGenerateMAC(xmlopt, &def->mac);
>>          def->mac_generated = true;
>>      }
>> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
>> index 073f298b5b66..e06bb200fc68 100644
>> --- a/src/util/virmacaddr.c
>> +++ b/src/util/virmacaddr.c
>> @@ -246,6 +246,11 @@ virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN])
>>      return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
>>  }
>>
>> +bool virMacAddrIsAllClear(const virMacAddr *addr)
>> +{
>> +    return !virMacAddrCmpRaw(addr, (const unsigned char[VIR_MAC_BUFLEN]){0});
>> +}
>> +
>>  void
>>  virMacAddrFree(virMacAddr *addr)
>>  {
>> diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
>> index f32b58805a61..7b9eb7443bd1 100644
>> --- a/src/util/virmacaddr.h
>> +++ b/src/util/virmacaddr.h
>> @@ -58,6 +58,7 @@ int virMacAddrParseHex(const char* str,
>>  bool virMacAddrIsUnicast(const virMacAddr *addr);
>>  bool virMacAddrIsMulticast(const virMacAddr *addr);
>>  bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
>> +bool virMacAddrIsAllClear(const virMacAddr *addr);
>>  void virMacAddrFree(virMacAddr *addr);
>>
>>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMacAddr, virMacAddrFree);
>
>Please expose the symbol in src/libvirt_private.syms too.
>

OK, done and pushed, thanks.

>Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
>Michal
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.