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; } + } +

Hmm, I didn't seem to put the parse statements before the foundIndex logic so this command is failing, I will fix that in next version.

...

+ /* 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