[PATCH] network: add modify-or-add feature to net-update
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@gmail.com> --- 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; + } + } + + /* 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)) { 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)) { 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) + 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; + } + 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)) { /* add to beginning/end of list */ if (VIR_INSERT_ELEMENT(def->portGroups, - 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->nPortGroups, def->nPortGroups, portgroup) < 0) goto cleanup; @@ -3144,7 +3208,9 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def, virNetworkDNSDef *dns = &def->dns; virNetworkDNSHostDef host = { 0 }; bool isAdd = (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_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST); int foundCt = 0; if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { @@ -3185,15 +3251,21 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def, if (isAdd) { if (foundCt > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("there is already at least one DNS HOST record with a matching field in network %1$s"), - def->name); + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is already at least one DNS HOST record with a matching field in network %1$s"), + def->name); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("DNS HOST records cannot be modified, only added or deleted")); goto cleanup; } /* add to beginning/end of list */ if (VIR_INSERT_ELEMENT(dns->hosts, - command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST) ? 0 : dns->nhosts, dns->nhosts, host) < 0) goto cleanup; } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { @@ -3240,7 +3312,9 @@ virNetworkDefUpdateDNSSrv(virNetworkDef *def, virNetworkDNSDef *dns = &def->dns; virNetworkDNSSrvDef srv = { 0 }; bool isAdd = (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_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST); int foundCt = 0; if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { @@ -3268,15 +3342,21 @@ virNetworkDefUpdateDNSSrv(virNetworkDef *def, if (isAdd) { if (foundCt > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("there is already at least one DNS SRV record matching all specified fields in network %1$s"), - def->name); + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is already at least one DNS SRV record matching all specified fields in network %1$s"), + def->name); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("DNS SRV records cannot be modified, only added or deleted")); goto cleanup; } /* add to beginning/end of list */ if (VIR_INSERT_ELEMENT(dns->srvs, - command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST) ? 0 : dns->nsrvs, dns->nsrvs, srv) < 0) goto cleanup; } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { @@ -3322,7 +3402,9 @@ virNetworkDefUpdateDNSTxt(virNetworkDef *def, virNetworkDNSDef *dns = &def->dns; virNetworkDNSTxtDef txt = { 0 }; bool isAdd = (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_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST); if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -3344,15 +3426,21 @@ virNetworkDefUpdateDNSTxt(virNetworkDef *def, if (isAdd) { if (foundIdx < dns->ntxts) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("there is already a DNS TXT record with name '%1$s' in network %2$s"), - txt.name, def->name); + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST)) + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is already a DNS TXT record with name '%1$s' in network %2$s"), + txt.name, def->name); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("DNS TXT records cannot be modified, only added or deleted")); goto cleanup; } /* add to beginning/end of list */ if (VIR_INSERT_ELEMENT(dns->txts, - command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST) ? 0 : dns->ntxts, dns->ntxts, txt) < 0) goto cleanup; } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 597e3d4530..c30305ac50 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1231,7 +1231,7 @@ 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 +1260,7 @@ 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.44.0
ping, it's been a while since i have put this out. On Sat, 16 Mar 2024 at 01:51, Abhiram Tilak <atp.exp@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.
Fixes: https://gitlab.com/libvirt/libvirt/-/issues/363 Signed-off-by: Abhiram Tilak <atp.exp@gmail.com> --- 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; + } + } + + /* 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)) {
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)) {
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) + 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; + } + 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)) {
/* add to beginning/end of list */ if (VIR_INSERT_ELEMENT(def->portGroups, - 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->nPortGroups, def->nPortGroups, portgroup) < 0) goto cleanup; @@ -3144,7 +3208,9 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def, virNetworkDNSDef *dns = &def->dns; virNetworkDNSHostDef host = { 0 }; bool isAdd = (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_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST); int foundCt = 0;
if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { @@ -3185,15 +3251,21 @@ virNetworkDefUpdateDNSHost(virNetworkDef *def, if (isAdd) {
if (foundCt > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("there is already at least one DNS HOST record with a matching field in network %1$s"), - def->name); + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is already at least one DNS HOST record with a matching field in network %1$s"), + def->name); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("DNS HOST records cannot be modified, only added or deleted")); goto cleanup; }
/* add to beginning/end of list */ if (VIR_INSERT_ELEMENT(dns->hosts, - command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST) ? 0 : dns->nhosts, dns->nhosts, host) < 0) goto cleanup; } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { @@ -3240,7 +3312,9 @@ virNetworkDefUpdateDNSSrv(virNetworkDef *def, virNetworkDNSDef *dns = &def->dns; virNetworkDNSSrvDef srv = { 0 }; bool isAdd = (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_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST); int foundCt = 0;
if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { @@ -3268,15 +3342,21 @@ virNetworkDefUpdateDNSSrv(virNetworkDef *def, if (isAdd) {
if (foundCt > 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("there is already at least one DNS SRV record matching all specified fields in network %1$s"), - def->name); + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is already at least one DNS SRV record matching all specified fields in network %1$s"), + def->name); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("DNS SRV records cannot be modified, only added or deleted")); goto cleanup; }
/* add to beginning/end of list */ if (VIR_INSERT_ELEMENT(dns->srvs, - command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST) ? 0 : dns->nsrvs, dns->nsrvs, srv) < 0) goto cleanup; } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { @@ -3322,7 +3402,9 @@ virNetworkDefUpdateDNSTxt(virNetworkDef *def, virNetworkDNSDef *dns = &def->dns; virNetworkDNSTxtDef txt = { 0 }; bool isAdd = (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_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_LAST);
if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -3344,15 +3426,21 @@ virNetworkDefUpdateDNSTxt(virNetworkDef *def, if (isAdd) {
if (foundIdx < dns->ntxts) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("there is already a DNS TXT record with name '%1$s' in network %2$s"), - txt.name, def->name); + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST)) + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is already a DNS TXT record with name '%1$s' in network %2$s"), + txt.name, def->name); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("DNS TXT records cannot be modified, only added or deleted")); goto cleanup; }
/* add to beginning/end of list */ if (VIR_INSERT_ELEMENT(dns->txts, - command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST || + command == VIR_NETWORK_UPDATE_COMMAND_MODIFY_OR_ADD_FIRST) ? 0 : dns->ntxts, dns->ntxts, txt) < 0) goto cleanup; } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 597e3d4530..c30305ac50 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1231,7 +1231,7 @@ 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 +1260,7 @@ 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.44.0
-- Abhiram
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@gmail.com> --- 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.
On Tue, 14 May 2024 at 18:33, Martin Kletzander <mkletzan@redhat.com> 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@gmail.com> --- 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
participants (3)
-
Abhiram Tilak -
atp exp
-
Martin Kletzander