[PATCH] network: allow "modify" option for DNS hostname

Adam Julis posted 1 patch 3 months, 3 weeks ago
Failed in applying to current master (apply log)
src/conf/network_conf.c                       | 28 ++++++++++++++-----
.../dns-host-modify-not-existing.xml          |  4 +++
.../dns-host-modify.xml                       |  5 ++++
.../nat-network-dns-hosts-modified.xml        | 28 +++++++++++++++++++
tests/networkxml2xmlupdatetest.c              |  9 ++++++
5 files changed, 67 insertions(+), 7 deletions(-)
create mode 100644 tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml
create mode 100644 tests/networkxml2xmlupdatein/dns-host-modify.xml
create mode 100644 tests/networkxml2xmlupdateout/nat-network-dns-hosts-modified.xml
[PATCH] network: allow "modify" option for DNS hostname
Posted by Adam Julis 3 months, 3 weeks ago
The "modify" command allows you to replace an existing record
(its hostname, sub-elements). IP address acts as the primary key.
If it is not found, the attempt ends with an error message. If
the XML contains a duplicate address, it will select the last
one.

Tests in networkxml2xmlupdatetest.c contain replacements of an
existing DNS-Host record and failure due to non-existing record.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/639
Signed-off-by: Adam Julis <ajulis@redhat.com>
---
 src/conf/network_conf.c                       | 28 ++++++++++++++-----
 .../dns-host-modify-not-existing.xml          |  4 +++
 .../dns-host-modify.xml                       |  5 ++++
 .../nat-network-dns-hosts-modified.xml        | 28 +++++++++++++++++++
 tests/networkxml2xmlupdatetest.c              |  9 ++++++
 5 files changed, 67 insertions(+), 7 deletions(-)
 create mode 100644 tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml
 create mode 100644 tests/networkxml2xmlupdatein/dns-host-modify.xml
 create mode 100644 tests/networkxml2xmlupdateout/nat-network-dns-hosts-modified.xml

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index f5ccf4bd12..2a541cd5b0 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -3138,19 +3138,13 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def,
                            unsigned int fflags G_GNUC_UNUSED)
 {
     size_t i, j, k;
-    int foundIdx = -1, ret = -1;
+    int foundIdx = -1, ret = -1, foundIdxModify = -1;
     virNetworkDNSDef *dns = &def->dns;
     virNetworkDNSHostDef host = { 0 };
     bool isAdd = (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST ||
                   command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST);
     int foundCt = 0;
 
-    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
-        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-                       _("DNS HOST records cannot be modified, only added or deleted"));
-        goto cleanup;
-    }
-
     if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0)
         goto cleanup;
 
@@ -3163,6 +3157,12 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def,
         if (virSocketAddrEqual(&host.ip, &dns->hosts[i].ip))
             foundThisTime = true;
 
+        /* modify option required index of matching ip-address, the loop under
+         * this comment could affect results of found index foundThisTime,
+         * so the foundIdxModify is there used instead */
+        if (foundThisTime)
+            foundIdxModify = i;
+
         /* when adding we want to only check duplicates of address since having
          * multiple addresses with the same hostname is a legitimate configuration */
         if (!isAdd) {
@@ -3213,6 +3213,20 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def,
         virNetworkDNSHostDefClear(&dns->hosts[foundIdx]);
         VIR_DELETE_ELEMENT(dns->hosts, foundIdx, dns->nhosts);
 
+    } else if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
+
+        if (foundCt == 0) {
+            virReportError(VIR_ERR_OPERATION_INVALID,
+                           _("couldn't locate a matching DNS HOST record in network %1$s"),
+                           def->name);
+            goto cleanup;
+        }
+
+        virNetworkDNSHostDefClear(&dns->hosts[foundIdxModify]);
+
+        memcpy(&dns->hosts[foundIdxModify], &host, sizeof(virNetworkDNSHostDef));
+        memset(&host, 0, sizeof(virNetworkDNSHostDef));
+
     } else {
         virNetworkDefUpdateUnknownCommand(command);
         goto cleanup;
diff --git a/tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml b/tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml
new file mode 100644
index 0000000000..357fccd110
--- /dev/null
+++ b/tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml
@@ -0,0 +1,4 @@
+<host ip='192.168.122.333'>
+    <hostname>shared</hostname>
+    <hostname>names</hostname>
+</host>
diff --git a/tests/networkxml2xmlupdatein/dns-host-modify.xml b/tests/networkxml2xmlupdatein/dns-host-modify.xml
new file mode 100644
index 0000000000..78b9fd88a6
--- /dev/null
+++ b/tests/networkxml2xmlupdatein/dns-host-modify.xml
@@ -0,0 +1,5 @@
+<host ip='192.168.122.2'>
+      <hostname>Another</hostname>
+      <hostname>decent</hostname>
+      <hostname>names</hostname>
+</host>
diff --git a/tests/networkxml2xmlupdateout/nat-network-dns-hosts-modified.xml b/tests/networkxml2xmlupdateout/nat-network-dns-hosts-modified.xml
new file mode 100644
index 0000000000..8fcaad15d1
--- /dev/null
+++ b/tests/networkxml2xmlupdateout/nat-network-dns-hosts-modified.xml
@@ -0,0 +1,28 @@
+<network>
+  <name>default</name>
+  <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid>
+  <forward dev='eth0' mode='nat'>
+    <interface dev='eth0'/>
+  </forward>
+  <bridge name='virbr0' stp='on' delay='0'/>
+  <dns forwardPlainNames='no'>
+    <host ip='192.168.122.122'>
+      <hostname>pudding</hostname>
+    </host>
+    <host ip='192.168.122.1'>
+      <hostname>host</hostname>
+      <hostname>gateway</hostname>
+    </host>
+    <host ip='192.168.122.2'>
+      <hostname>Another</hostname>
+      <hostname>decent</hostname>
+      <hostname>names</hostname>
+    </host>
+    <host ip='fd8f:1391:3a82:150::c0a8:9603'>
+      <hostname>shared</hostname>
+      <hostname>names</hostname>
+    </host>
+  </dns>
+  <ip address='192.168.122.1' netmask='255.255.255.0'>
+  </ip>
+</network>
diff --git a/tests/networkxml2xmlupdatetest.c b/tests/networkxml2xmlupdatetest.c
index afe2b1f574..383cbf85ce 100644
--- a/tests/networkxml2xmlupdatetest.c
+++ b/tests/networkxml2xmlupdatetest.c
@@ -276,6 +276,15 @@ mymain(void)
             "nat-network-dns-hosts",
             "nat-network-no-hosts",
             VIR_NETWORK_UPDATE_COMMAND_DELETE);
+    DO_TEST("modify-dns-host",
+            "dns-host-modify",
+            "nat-network-dns-hosts",
+            "nat-network-dns-hosts-modified",
+            VIR_NETWORK_UPDATE_COMMAND_MODIFY);
+    DO_TEST_FAIL("modify-dns-host-not-existing",
+                 "dns-host-modify-not-existing",
+                 "nat-network-dns-hosts",
+                 VIR_NETWORK_UPDATE_COMMAND_MODIFY);
 
 
     section = VIR_NETWORK_SECTION_DNS_TXT;
-- 
2.45.2
Re: [PATCH] network: allow "modify" option for DNS hostname
Posted by Michal Prívozník 3 months, 3 weeks ago
On 7/9/24 17:23, Adam Julis wrote:
> The "modify" command allows you to replace an existing record
> (its hostname, sub-elements). IP address acts as the primary key.
> If it is not found, the attempt ends with an error message. If
> the XML contains a duplicate address, it will select the last
> one.
> 
> Tests in networkxml2xmlupdatetest.c contain replacements of an
> existing DNS-Host record and failure due to non-existing record.
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/639
> Signed-off-by: Adam Julis <ajulis@redhat.com>
> ---
>  src/conf/network_conf.c                       | 28 ++++++++++++++-----
>  .../dns-host-modify-not-existing.xml          |  4 +++
>  .../dns-host-modify.xml                       |  5 ++++
>  .../nat-network-dns-hosts-modified.xml        | 28 +++++++++++++++++++
>  tests/networkxml2xmlupdatetest.c              |  9 ++++++
>  5 files changed, 67 insertions(+), 7 deletions(-)
>  create mode 100644 tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml
>  create mode 100644 tests/networkxml2xmlupdatein/dns-host-modify.xml
>  create mode 100644 tests/networkxml2xmlupdateout/nat-network-dns-hosts-modified.xml
> 
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index f5ccf4bd12..2a541cd5b0 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -3138,19 +3138,13 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def,
>                             unsigned int fflags G_GNUC_UNUSED)
>  {
>      size_t i, j, k;
> -    int foundIdx = -1, ret = -1;
> +    int foundIdx = -1, ret = -1, foundIdxModify = -1;

Nitpick, we prefer each variable to be declared on its own line.

>      virNetworkDNSDef *dns = &def->dns;
>      virNetworkDNSHostDef host = { 0 };
>      bool isAdd = (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST ||
>                    command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST);
>      int foundCt = 0;
>  
> -    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -                       _("DNS HOST records cannot be modified, only added or deleted"));
> -        goto cleanup;
> -    }
> -
>      if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0)
>          goto cleanup;
>  
> @@ -3163,6 +3157,12 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def,
>          if (virSocketAddrEqual(&host.ip, &dns->hosts[i].ip))
>              foundThisTime = true;
>  
> +        /* modify option required index of matching ip-address, the loop under
> +         * this comment could affect results of found index foundThisTime,
> +         * so the foundIdxModify is there used instead */
> +        if (foundThisTime)
> +            foundIdxModify = i;
> +
>          /* when adding we want to only check duplicates of address since having
>           * multiple addresses with the same hostname is a legitimate configuration */
>          if (!isAdd) {
> @@ -3213,6 +3213,20 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def,
>          virNetworkDNSHostDefClear(&dns->hosts[foundIdx]);
>          VIR_DELETE_ELEMENT(dns->hosts, foundIdx, dns->nhosts);
>  
> +    } else if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
> +
> +        if (foundCt == 0) {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                           _("couldn't locate a matching DNS HOST record in network %1$s"),
> +                           def->name);
> +            goto cleanup;
> +        }
> +
> +        virNetworkDNSHostDefClear(&dns->hosts[foundIdxModify]);
> +
> +        memcpy(&dns->hosts[foundIdxModify], &host, sizeof(virNetworkDNSHostDef));
> +        memset(&host, 0, sizeof(virNetworkDNSHostDef));
> +
>      } else {
>          virNetworkDefUpdateUnknownCommand(command);
>          goto cleanup;
> diff --git a/tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml b/tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml
> new file mode 100644
> index 0000000000..357fccd110
> --- /dev/null
> +++ b/tests/networkxml2xmlupdatein/dns-host-modify-not-existing.xml
> @@ -0,0 +1,4 @@
> +<host ip='192.168.122.333'>
> +    <hostname>shared</hostname>
> +    <hostname>names</hostname>
> +</host>

For XMLs we prefer two spaces of indentation. In VIM you can achieve
this by setting :set shiftwidth=2

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

Michal