4:21 a.m.
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>
---
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
5:49 p.m.
ping, it's been a while since i have put this out.
On Sat, 16 Mar 2024 at 01:51, Abhiram Tilak <atp.exp(a)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(a)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
9:03 p.m.
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>
---
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.
5:56 p.m.
On Tue, 14 May 2024 at 18:33, Martin Kletzander <mkletzan(a)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(a)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
115
days inactive
176
days old
3 comments
3 participants
participants (3)
-
Abhiram Tilak -
atp exp
-
Martin Kletzander