On Wed, 15 May 2024 at 15:43, Abhiram Tilak <atp.exp(a)gmail.com&gt; 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(a)gmail.com&gt; > --- > 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