On Tue, 14 May 2024 at 18:33, Martin Kletzander <mkletzan@xxxxxxxxxx> 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 <atp.exp@xxxxxxxxx>
>---
> 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.
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.
-- 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
