On Tue, 14 May 2024 at 18:33, Martin Kletzander <[email protected]> wrote:
> 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 <[email protected]> > >--- > > 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. > I agree, infact in my opinion the current implementation also involves a lot of duplication, I will try to come up with a better solution. I will take some inspiration from the `PortGroup`'s implementation of using a `foundName`. >+ /* 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. > I agree, modify-or-add doesn't make sense without modify being allowed. Yes, the DNS options can be changed to handle `modify` too, and that will go inside a separate patch. I can put it out maybe in a day or two. > > 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. > I think just changing the `command`, would help make the code a lot easier to read in many of these instances. Usually we rarely change the function parameters, but in this case it seems harmless to do so. -- Abhiram
