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....@gmail.com> wrote: > > On Wed, 15 May 2024 at 15:43, Abhiram Tilak <atp....@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....@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
