nvm, the command was not working due to a different reason.
The indexes used in delete and modify were still `i` need to
update that.



On Tue, 21 May 2024 at 15:31, atp exp <atp.exp@gmail.com> wrote:

On Wed, 15 May 2024 at 15:43, Abhiram Tilak <atp.exp@gmail.com> wrote:
The current way of updating a network configuration uses `virsh
net-update` to add, delete or modify entries. But with such a mechansim
one should know if an entry with current info already exists. Adding
modify-or-add option automatically performs either modify or add
depending on the current state.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/363
Signed-off-by: Abhiram Tilak <atp.exp@gmail.com>
---
Changes in v2:
  - Removed the modify-or-add functionality for sections
    where modify is not applicable.
  - Changed the existing implementation of `UpdateIPDHCPHost`,
    to avoid code duplication.
  - Changed the implementation of modify-or-delete to reassign
    the `command` variable instead of using multiple nested conditions.

 docs/manpages/virsh.rst           |   5 +-
 include/libvirt/libvirt-network.h |  12 +--
 src/conf/network_conf.c           | 134 +++++++++++++++++++++---------
 tools/virsh-network.c             |   6 +-
 4 files changed, 110 insertions(+), 47 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index 115b802c45..0da3827f6b 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -5908,7 +5908,10 @@ changes optionally taking effect immediately, without needing to
 destroy and re-start the network.

 *command* is one of "add-first", "add-last", "add" (a synonym for
-add-last), "delete", or "modify".
+add-last), "delete", "modify", "modify-or-add" (modify + add-last), or
+"modify-or-add-first". The 'modify-or-add*' commands perform modify or
+add operation depending on the given state, and can be useful for
+scripting.

 *section* is one of "bridge", "domain", "ip", "ip-dhcp-host",
 "ip-dhcp-range", "forward", "forward-interface", "forward-pf",
diff --git a/include/libvirt/libvirt-network.h b/include/libvirt/libvirt-network.h
index 58591be7ac..bb4468b160 100644
--- a/include/libvirt/libvirt-network.h
+++ b/include/libvirt/libvirt-network.h
@@ -176,11 +176,13 @@ int                     virNetworkUndefine      (virNetworkPtr network);
  * Since: 0.10.2
  */
 typedef enum {
-    VIR_NETWORK_UPDATE_COMMAND_NONE      = 0, /* invalid (Since: 0.10.2) */
-    VIR_NETWORK_UPDATE_COMMAND_MODIFY    = 1, /* modify an existing element (Since: 0.10.2) */
-    VIR_NETWORK_UPDATE_COMMAND_DELETE    = 2, /* delete an existing element (Since: 0.10.2) */
-    VIR_NETWORK_UPDATE_COMMAND_ADD_LAST  = 3, /* add an element at end of list (Since: 0.10.2) */
-    VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST = 4, /* add an element at start of list (Since: 0.10.2) */
+    VIR_NETWORK_UPDATE_COMMAND_NONE             = 0, /* invalid (Since: 0.10.2) */
+    VIR_NETWORK_UPDATE_COMMAND_MODIFY           = 1, /* modify an existing element (Since: 0.10.2) */
+    VIR_NETWORK_UPDATE_COMMAND_DELETE           = 2, /* delete an existing element (Since: 0.10.2) */
+    VIR_NETWORK_UPDATE_COMMAND_ADD_LAST         = 3, /* add an element at end of list (Since: 0.10.2) */
+    VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST        = 4, /* add an element at start of list (Since: 0.10.2) */
+    VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST  = 5, /* conditionally modify or add an element at end (Since: 10.4.0) */
+    VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST = 6, /* conditionally modify or add an element at start (Since: 10.4.0) */
 # ifdef VIR_ENUM_SENTINELS
     VIR_NETWORK_UPDATE_COMMAND_LAST /* (Since: 0.10.2) */
 # endif
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index cc92ed0b03..a7c3dea163 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2720,6 +2720,7 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
     virNetworkIPDef *ipdef = virNetworkIPDefByIndex(def, parentIndex);
     virNetworkDHCPHostDef host = { 0 };
     bool partialOkay = (command == VIR_NETWORK_UPDATE_COMMAND_DELETE);
+    int foundMatchedEntry = -1, foundExactEntry = -1;

     if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0)
         goto cleanup;
@@ -2740,22 +2741,47 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
         goto cleanup;
     }

-    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {
+    /* check if the entry already exsits */
+    for (i = 0; i < ipdef->nhosts; i++) {

-        /* search for the entry with this (ip|mac|name),
-         * and update the IP+(mac|name) */
-        for (i = 0; i < ipdef->nhosts; i++) {
-            if ((host.mac && ipdef->hosts[i].mac &&
-                 !virMacAddrCompare(host.mac, ipdef->hosts[i].mac)) ||
-                (VIR_SOCKET_ADDR_VALID(&host.ip) &&
-                 virSocketAddrEqual(&host.ip, &ipdef->hosts[i].ip)) ||
-                (host.name &&
-                 STREQ_NULLABLE(host.name, ipdef->hosts[i].name))) {
-                break;
-            }
+        /* try to match any of (ip|mac|name) attributes */
+        if ((host.mac && ipdef->hosts[i].mac &&
+             !virMacAddrCompare(host.mac, ipdef->hosts[i].mac)) ||
+            (VIR_SOCKET_ADDR_VALID(&host.ip) &&
+             virSocketAddrEqual(&host.ip, &ipdef->hosts[i].ip)) ||
+            (host.name &&
+             STREQ_NULLABLE(host.name, ipdef->hosts[i].name))) {
+            foundMatchedEntry = i;
+        }
+
+        /* find exact entry - all specified attributes must match */
+        if ((!host.mac || !ipdef->hosts[i].mac ||
+             !virMacAddrCompare(host.mac, ipdef->hosts[i].mac)) &&
+            (!host.name ||
+             STREQ_NULLABLE(host.name, ipdef->hosts[i].name)) &&
+            (!VIR_SOCKET_ADDR_VALID(&host.ip) ||
+             virSocketAddrEqual(&host.ip, &ipdef->hosts[i].ip))) {
+            foundExactEntry = i;
+            break;
         }
+    }
+

+    /* modify-or-add: convert command to add or modify based on foundEntry */
+    if (foundEntry == -1) {
+        if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST)
+            command = VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST;
+        else if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST)
+            command = VIR_NETWORK_UPDATE_COMMAND_ADD_LAST;
+    } else if (foundEntry >= 0) {
+        if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST ||
+            command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST)
+            command = VIR_NETWORK_UPDATE_COMMAND_MODIFY;
+    }
+
+    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) {

-        if (i == ipdef->nhosts) {
+        /* log error if no such entry exists to be modified */
+        if (foundMatchedEntry == -1) {
             g_autofree char *ip = virSocketAddrFormat(&host.ip);
             virReportError(VIR_ERR_OPERATION_INVALID,
                            _("couldn't locate an existing dhcp host entry with \"mac='%1$s'\" \"name='%2$s'\" \"ip='%3$s'\" in network '%4$s'"),
@@ -2779,21 +2805,13 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
             goto cleanup;

         /* log error if an entry with same name/address/ip already exists */
-        for (i = 0; i < ipdef->nhosts; i++) {
-            if ((host.mac && ipdef->hosts[i].mac &&
-                 !virMacAddrCompare(host.mac, ipdef->hosts[i].mac)) ||
-                (host.name &&
-                 STREQ_NULLABLE(host.name, ipdef->hosts[i].name)) ||
-                (VIR_SOCKET_ADDR_VALID(&host.ip) &&
-                 virSocketAddrEqual(&host.ip, &ipdef->hosts[i].ip))) {
-                g_autofree char *ip = virSocketAddrFormat(&host.ip);
+        if (foundMatchedEntry >= 0) {
+            virReportError(VIR_ERR_OPERATION_INVALID,
+                           _("there is an existing dhcp host entry in network '%1$s' that matches \"<host mac='%2$s' name='%3$s' ip='%4$s'/>\""),
+                           def->name, host.mac ? host.mac : _("unknown"),
+                           host.name, ip ? ip : _("unknown"));
+            goto cleanup;

-                virReportError(VIR_ERR_OPERATION_INVALID,
-                               _("there is an existing dhcp host entry in network '%1$s' that matches \"<host mac='%2$s' name='%3$s' ip='%4$s'/>\""),
-                               def->name, host.mac ? host.mac : _("unknown"),
-                               host.name, ip ? ip : _("unknown"));
-                goto cleanup;
-            }
         }

         /* add to beginning/end of list */
@@ -2804,18 +2822,8 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def,
             goto cleanup;
     } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) {

-        /* find matching entry - all specified attributes must match */
-        for (i = 0; i < ipdef->nhosts; i++) {
-            if ((!host.mac || !ipdef->hosts[i].mac ||
-                 !virMacAddrCompare(host.mac, ipdef->hosts[i].mac)) &&
-                (!host.name ||
-                 STREQ_NULLABLE(host.name, ipdef->hosts[i].name)) &&
-                (!VIR_SOCKET_ADDR_VALID(&host.ip) ||
-                 virSocketAddrEqual(&host.ip, &ipdef->hosts[i].ip))) {
-                break;
-            }
-        }
-        if (i == ipdef->nhosts) {
+        /* log error if there is no entry with exact match*/
+        if (foundExactEntry == -1) {
             virReportError(VIR_ERR_OPERATION_INVALID,
                            _("couldn't locate a matching dhcp host entry in network '%1$s'"),
                            def->name);
@@ -2865,6 +2873,14 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDef *def,
         return -1;
     }

+    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST ||
+        command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST) {
+
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("dhcp ranges cannot be modified, use add command instead of modify-or-add"));
+        return -1;
+    }
+
     if (virNetworkDHCPRangeDefParseXML(def->name, ipdef, ctxt->node, &range) < 0)
         return -1;

@@ -2964,6 +2980,13 @@ virNetworkDefUpdateForwardInterface(virNetworkDef *def,
         goto cleanup;
     }

+    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST ||
+        command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("forward interface entries cannot be modified, use add command instead of modify-or-add"));
+        goto cleanup;
+    }
+
     /* parsing this is so simple that it doesn't have its own function */
     iface.type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV;
     if (!(iface.device.dev = virXMLPropString(ctxt->node, "dev"))) {
@@ -3085,6 +3108,18 @@ virNetworkDefUpdatePortGroup(virNetworkDef *def,
         goto cleanup;
     }

+    /* modify-or-add: convert command to add or modify based on foundName */
+    if (foundName == -1) {
+        if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST)
+            command = VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST;
+        else if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST)
+            command = VIR_NETWORK_UPDATE_COMMAND_ADD_LAST;
+    } else if (foundName >= 0) {
+        if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST ||
+            command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST)
+            command = VIR_NETWORK_UPDATE_COMMAND_MODIFY;
+    }
+
     /* if there is already a different default, we can't make this
      * one the default.
      */
@@ -3153,6 +3188,13 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def,
         goto cleanup;
     }

+    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST ||
+        command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("DNS HOST records cannot be modified, use add instead of modify-or-add"));
+        goto cleanup;
+    }
+
     if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0)
         goto cleanup;

@@ -3249,6 +3291,13 @@ virNetworkDefUpdateDNSSrv(virNetworkDef *def,
         goto cleanup;
     }

+    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST ||
+        command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("DNS SRV records cannot be modified, use add command instead of modify-or-add"));
+        goto cleanup;
+    }
+
     if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "srv") < 0)
         goto cleanup;

@@ -3330,6 +3379,13 @@ virNetworkDefUpdateDNSTxt(virNetworkDef *def,
         goto cleanup;
     }

+    if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_FIRST ||
+        command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_ADD_LAST) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("DNS TXT records cannot be modified, use add command instead of modify-or-add"));
+        goto cleanup;
+    }
+
     if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "txt") < 0)
         goto cleanup;

diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index 597e3d4530..d7dc5df5a8 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -1231,7 +1231,8 @@ static const vshCmdOptDef opts_network_update[] = {
      .positional = true,
      .required = true,
      .completer = virshNetworkUpdateCommandCompleter,
-     .help = N_("type of update (add-first, add-last (add), delete, or modify)")
+     .help = N_("type of update (add-first, add-last (add), delete, modify,"
+                "modify-or-add, or modify-or-add-first)")
     },
     {.name = "section",
      .type = VSH_OT_STRING,
@@ -1260,7 +1261,8 @@ static const vshCmdOptDef opts_network_update[] = {

 VIR_ENUM_IMPL(virshNetworkUpdateCommand,
               VIR_NETWORK_UPDATE_COMMAND_LAST,
-              "none", "modify", "delete", "add-last", "add-first");
+              "none", "modify", "delete", "add-last", "add-first", "modify-or-add",
+              "modify-or-add-first");

 VIR_ENUM_IMPL(virshNetworkSection,
               VIR_NETWORK_SECTION_LAST,
--
2.42.1



--
Abhiram


--
Abhiram