Re: [PATCH] network: add modify-or-add feature to net-update

On Sat, Mar 16, 2024 at 01:43:52AM +0530, Abhiram Tilak 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. >Fixes: https://gitlab.com/libvirt/libvirt/-/issues/363 >Signed-off-by: Abhiram Tilak <atp.exp(a)gmail.com&gt; >--- > docs/manpages/virsh.rst | 5 +- > include/libvirt/libvirt-network.h | 2 + > src/conf/network_conf.c | 148 ++++++++++++++++++++++++------ > tools/virsh-network.c | 4 +- > 4 files changed, 126 insertions(+), 33 deletions(-) > >diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst >index 115b802c45..dc91ba895c 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), >+"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..a6e132f407 100644 >--- a/include/libvirt/libvirt-network.h >+++ b/include/libvirt/libvirt-network.h >@@ -181,6 +181,8 @@ typedef enum { > 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_OR_ADD_LAST = 5, /* if exists modify or add an element at end of list (Since: 0.10.2) */ >+ VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST = 6, /* if exists modify or add an element at start of list (Since: 0.10.2) */ > # 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..2835395385 100644 >--- a/src/conf/network_conf.c >+++ b/src/conf/network_conf.c >@@ -2721,6 +2721,9 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def, > virNetworkDHCPHostDef host = { 0 }; > bool partialOkay = (command == VIR_NETWORK_UPDATE_COMMAND_DELETE); > >+ /* added for modify-or-add feature */ >+ bool modified = false; >+ > if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "host") < 0) > goto cleanup; > >@@ -2826,7 +2829,34 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDef *def, > virNetworkDHCPHostDefClear(&ipdef->hosts[i]); > VIR_DELETE_ELEMENT(ipdef->hosts, i, ipdef->nhosts); > >- } else { >+ } else if ((command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST) || >+ (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST)) { >+ >+ /* find entries with matching name/address/ip */ >+ 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))) { >+ >+ modified = true; >+ break; >+ } >+ } >+ This duplicates lot of the existing code. Wouldn't it be nicer if you used the code path in VIR_NETWORK_UPDATE_COMMAND_MODIFY that fails if the item cannot be found and changed it to ADD(_FIRST) if it is one of these commands? Of course the detection of unknown command would have to be rewritten, but it might look nicer and remove the error-prone nature of duplicating the code. Another option would be to extract the duplicated parts into another function, but they don't seem _that_ large.

+ /* if element is found then modify, or else add to beginning/end of list */ >+ if (modified) { >+ virNetworkDHCPHostDefClear(&ipdef->hosts[i]); >+ ipdef->hosts[i] = host; >+ memset(&host, 0, sizeof(host)); >+ } else if (VIR_INSERT_ELEMENT(ipdef->hosts, >+ command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST >+ ? 0 : ipdef->nhosts, >+ ipdef->nhosts, host) < 0) >+ goto cleanup; >+ } else { > virNetworkDefUpdateUnknownCommand(command); > goto cleanup; > } >@@ -2885,7 +2915,9 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDef *def, > } > > if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || >- (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { >+ (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) || >+ (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST) || >+ (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST)) { > With the DHCP range it is a bit trickier. There could be an issue if someone thinks that modify-or-add will modify the existing range, but due to the nature of the IP ranges being impossible to detect whether an user wants to modify or add them, I would rather see this error out for modify-or-add(-first). Similarly with the UpdateForwardInterface. The DNS options of Host, Srv, and Txt could be updated to handle the "modify" use case, and only after that would it make sense for modify-or-add* to be implemented there too, IMHO.

> if (virNetworkDefUpdateCheckMultiDHCP(def, ipdef) < 0) > return -1; >@@ -2894,17 +2926,24 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDef *def, > g_autofree char *startip = virSocketAddrFormat(&range.addr.start); > g_autofree char *endip = virSocketAddrFormat(&range.addr.end); > >- virReportError(VIR_ERR_OPERATION_INVALID, >- _("there is an existing dhcp range entry in network '%1$s' that matches \"<range start='%2$s' end='%3$s'/>\""), >- def->name, >- startip ? startip : "unknown", >- endip ? endip : "unknown"); >+ >+ if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || >+ (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) >+ virReportError(VIR_ERR_OPERATION_INVALID, >+ _("there is an existing dhcp range entry in network '%1$s' that matches \"<range start='%2$s' end='%3$s'/>\""), >+ def->name, >+ startip ? startip : "unknown", >+ endip ? endip : "unknown"); >+ else >+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", >+ _("dhcp ranges cannot be modified, only added or deleted")); > return -1; > } > > /* add to beginning/end of list */ > if (VIR_INSERT_ELEMENT(ipdef->ranges, >- command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST >+ (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST || >+ command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST) > ? 0 : ipdef->nranges, > ipdef->nranges, range) < 0) > return -1; >@@ -2981,18 +3020,26 @@ virNetworkDefUpdateForwardInterface(virNetworkDef *def, > } > > if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || >- (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { >+ (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) || >+ (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST) || >+ (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST)) { the extra parentheses are not needed ... > > if (i < def->forward.nifs) { >- virReportError(VIR_ERR_OPERATION_INVALID, >- _("there is an existing interface entry in network '%1$s' that matches \"<interface dev='%2$s'>\""), >- def->name, iface.device.dev); >+ if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) || >+ command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) ... and you're mixing the usage of them resulting in less readable code. >+ virReportError(VIR_ERR_OPERATION_INVALID, >+ _("there is an existing interface entry in network '%1$s' that matches \"<interface dev='%2$s'>\""), >+ def->name, iface.device.dev); >+ else >+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", >+ _("forward interface entries cannot be modified, only added or deleted")); > goto cleanup; > } > > /* add to beginning/end of list */ > if (VIR_INSERT_ELEMENT(def->forward.ifs, >- command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST >+ (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST || >+ command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST) > ? 0 : def->forward.nifs, > def->forward.nifs, iface) < 0) > goto cleanup; >@@ -3056,6 +3103,9 @@ virNetworkDefUpdatePortGroup(virNetworkDef *def, > int ret = -1; > virPortGroupDef portgroup = { 0 }; > >+ /* added for modify-or-add feature */ >+ bool modified = false; >+ > if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "portgroup") < 0) > goto cleanup; > >@@ -3097,6 +3147,17 @@ virNetworkDefUpdatePortGroup(virNetworkDef *def, > goto cleanup; > } > >+ /* modify found entries for modify-or-add command */ >+ if (foundName >= 0 && (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST || >+ command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST)) { >+ >+ /* replace existing entry */ >+ virPortGroupDefClear(&def->portGroups[foundName]); >+ def->portGroups[foundName] = portgroup; >+ memset(&portgroup, 0, sizeof(portgroup)); >+ modified = true; another code duplication, this one is not that bad though >+ } >+ > if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { > > /* replace existing entry */ >@@ -3105,11 +3166,14 @@ virNetworkDefUpdatePortGroup(virNetworkDef *def, > memset(&portgroup, 0, sizeof(portgroup)); > > } else if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || >- (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { >+ (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) || >+ (!modified && command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST) || >+ (!modified && command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST)) { > Again, these could be easier if you changed the `command` when the entry was not found instead of expanding all the conditions. Other than the tremendous amount of complicated conditions it looks fine.