- Introduces virNetworkObjGetMetadata() and
virNetworkObjSetMetadata().
- These functions implement common behaviour that can be reused by
network drivers that use the virNetworkObj struct.
- Also adds helper functions that resolve the live/persistent state of
the network before setting metadata.
Signed-off-by: K Shiva Kiran <shiva_kr@riseup.net>
---
src/conf/virnetworkobj.c | 325 +++++++++++++++++++++++++++++++++++++++
src/conf/virnetworkobj.h | 17 ++
2 files changed, 342 insertions(+)
diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index b8b86da06f..41d0a1d971 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -1822,3 +1822,328 @@ virNetworkObjLoadAllPorts(virNetworkObj *net,
return 0;
}
+
+
+/**
+ * virNetworkObjUpdateModificationImpact:
+ *
+ * @net: network object
+ * @flags: flags to update the modification impact on
+ *
+ * Resolves virNetworkUpdateFlags in @flags so that they correctly
+ * apply to the actual state of @net. @flags may be modified after call to this
+ * function.
+ *
+ * Returns 0 on success if @flags point to a valid combination for @net or -1 on
+ * error.
+ */
+static int
+virNetworkObjUpdateModificationImpact(virNetworkObj *net,
+ unsigned int *flags)
+{
+ bool isActive = virNetworkObjIsActive(net);
+
+ if ((*flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE | VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
+ VIR_NETWORK_UPDATE_AFFECT_CURRENT) {
+ if (isActive)
+ *flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE;
+ else
+ *flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG;
+ }
+
+ if (!isActive && (*flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("network is not running"));
+ return -1;
+ }
+
+ if (!net->persistent && (*flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("transient networks do not have any "
+ "persistent config"));
+ return -1;
+ }
+
+ return 0;
+}
+
+
+/**
+ * virNetworkObjGetDefs:
+ *
+ * @net: network object
+ * @flags: for virNetworkUpdateFlags
+ * @liveDef: Set the pointer to the live definition of @net.
+ * @persDef: Set the pointer to the config definition of @net.
+ *
+ * Helper function to resolve @flags and retrieve correct network pointer
+ * objects. This function should be used only when the network driver
+ * creates net->newDef once the network has started.
+ *
+ * If @liveDef or @persDef are set it implies that @flags request modification
+ * thereof.
+ *
+ * Returns 0 on success and sets @liveDef and @persDef; -1 if @flags are
+ * inappropriate.
+ */
+static int
+virNetworkObjGetDefs(virNetworkObj *net,
+ unsigned int flags,
+ virNetworkDef **liveDef,
+ virNetworkDef **persDef)
+{
+ if (liveDef)
+ *liveDef = NULL;
+
+ if (persDef)
+ *persDef = NULL;
+
+ if (virNetworkObjUpdateModificationImpact(net, &flags) < 0)
+ return -1;
+
+ if (virNetworkObjIsActive(net)) {
+ if (liveDef && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE))
+ *liveDef = net->def;
+
+ if (persDef && (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG))
+ *persDef = net->newDef;
+ } else {
+ if (persDef)
+ *persDef = net->def;
+ }
+
+ return 0;
+}
+
+
+/**
+ * virNetworkObjGetOneDefState:
+ *
+ * @net: Network object
+ * @flags: for virNetworkUpdateFlags
+ * @live: set to true if live config was returned (may be omitted)
+ *
+ * Helper function to resolve @flags and return the correct network pointer
+ * object. This function returns one of @net->def or @net->persistentDef
+ * according to @flags. @live is set to true if the live net config will be
+ * returned. This helper should be used only in APIs that guarantee
+ * that @flags contains exactly one of VIR_NETWORK_UPDATE_AFFECT_LIVE or
+ * VIR_NETWORK_UPDATE_AFFECT_CONFIG and not both.
+ *
+ * Returns the correct definition pointer or NULL on error.
+ */
+static virNetworkDef *
+virNetworkObjGetOneDefState(virNetworkObj *net,
+ unsigned int flags,
+ bool *live)
+{
+ if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE &&
+ flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) {
+ virReportInvalidArg(flags, "%s",
+ _("Flags 'VIR_NETWORK_UPDATE_AFFECT_LIVE' and "
+ "'VIR_NETWORK_UPDATE_AFFECT_CONFIG' are mutually "
+ "exclusive"));
+ return NULL;
+ }
+
+ if (virNetworkObjUpdateModificationImpact(net, &flags) < 0)
+ return NULL;
+
+ if (live)
+ *live = flags & VIR_NETWORK_UPDATE_AFFECT_LIVE;
+
+ if (virNetworkObjIsActive(net) && flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG)
+ return net->newDef;
+
+ return net->def;
+}
+
+
+/**
+ * virNetworkObjGetOneDef:
+ *
+ * @net: Network object
+ * @flags: for virNetworkUpdateFlags
+ *
+ * Helper function to resolve @flags and return the correct network pointer
+ * object. This function returns one of @net->def or @net->persistentDef
+ * according to @flags. This helper should be used only in APIs that guarantee
+ * that @flags contains exactly one of VIR_NETWORK_UPDATE_AFFECT_LIVE or
+ * VIR_NETWORK_UPDATE_AFFECT_CONFIG and not both.
+ *
+ * Returns the correct definition pointer or NULL on error.
+ */
+static virNetworkDef *
+virNetworkObjGetOneDef(virNetworkObj *net,
+ unsigned int flags)
+{
+ return virNetworkObjGetOneDefState(net, flags, NULL);
+}
+
+
+char *
+virNetworkObjGetMetadata(virNetworkObj *net,
+ int type,
+ const char *uri,
+ unsigned int flags)
+{
+ virNetworkDef *def;
+ char *ret = NULL;
+
+ virCheckFlags(VIR_NETWORK_UPDATE_AFFECT_LIVE |
+ VIR_NETWORK_UPDATE_AFFECT_CONFIG, NULL);
+
+ if (type >= VIR_NETWORK_METADATA_LAST) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("unknown metadata type '%1$d'"), type);
+ return NULL;
+ }
+
+ if (!(def = virNetworkObjGetOneDef(net, flags)))
+ return NULL;
+
+ switch ((virNetworkMetadataType) type) {
+ case VIR_NETWORK_METADATA_DESCRIPTION:
+ ret = g_strdup(def->description);
+ break;
+
+ case VIR_NETWORK_METADATA_TITLE:
+ ret = g_strdup(def->title);
+ break;
+
+ case VIR_NETWORK_METADATA_ELEMENT:
+ if (!def->metadata)
+ break;
+
+ if (virXMLExtractNamespaceXML(def->metadata, uri, &ret) < 0)
+ return NULL;
+ break;
+
+ case VIR_NETWORK_METADATA_LAST:
+ break;
+ }
+
+ if (!ret)
+ virReportError(VIR_ERR_NO_NETWORK_METADATA, "%s",
+ _("Requested metadata element is not present"));
+
+ return ret;
+}
+
+
+static int
+virNetworkDefSetMetadata(virNetworkDef *def,
+ int type,
+ const char *metadata,
+ const char *key,
+ const char *uri)
+{
+ g_autoptr(xmlDoc) doc = NULL;
+ xmlNodePtr old;
+ g_autoptr(xmlNode) new = NULL;
+
+ if (type >= VIR_NETWORK_METADATA_LAST) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("unknown metadata type '%1$d'"), type);
+ return -1;
+ }
+
+ switch ((virNetworkMetadataType) type) {
+ case VIR_NETWORK_METADATA_DESCRIPTION:
+ g_clear_pointer(&def->description, g_free);
+
+ if (STRNEQ_NULLABLE(metadata, ""))
+ def->description = g_strdup(metadata);
+ break;
+
+ case VIR_NETWORK_METADATA_TITLE:
+ g_clear_pointer(&def->title, g_free);
+
+ if (STRNEQ_NULLABLE(metadata, ""))
+ def->title = g_strdup(metadata);
+ break;
+
+ case VIR_NETWORK_METADATA_ELEMENT:
+ if (metadata) {
+
+ /* parse and modify the xml from the user */
+ if (!(doc = virXMLParseStringCtxt(metadata, _("(metadata_xml)"), NULL)))
+ return -1;
+
+ if (virXMLInjectNamespace(doc->children, uri, key) < 0)
+ return -1;
+
+ /* create the root node if needed */
+ if (!def->metadata)
+ def->metadata = virXMLNewNode(NULL, "metadata");
+
+ if (!(new = xmlCopyNode(doc->children, 1))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to copy XML node"));
+ return -1;
+ }
+ }
+
+ /* remove possible other nodes sharing the namespace */
+ while ((old = virXMLFindChildNodeByNs(def->metadata, uri))) {
+ xmlUnlinkNode(old);
+ xmlFreeNode(old);
+ }
+
+ if (new) {
+ if (!(xmlAddChild(def->metadata, new))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to add metadata to XML document"));
+ return -1;
+ }
+ new = NULL;
+ }
+ break;
+
+ case VIR_NETWORK_METADATA_LAST:
+ break;
+ }
+
+ return 0;
+}
+
+
+int
+virNetworkObjSetMetadata(virNetworkObj *net,
+ int type,
+ const char *metadata,
+ const char *key,
+ const char *uri,
+ virNetworkXMLOption *xmlopt,
+ const char *stateDir,
+ const char *configDir,
+ unsigned int flags)
+{
+ virNetworkDef *def;
+ virNetworkDef *persistentDef;
+
+ virCheckFlags(VIR_NETWORK_UPDATE_AFFECT_LIVE |
+ VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1);
+
+ if (virNetworkObjGetDefs(net, flags, &def, &persistentDef) < 0)
+ return -1;
+
+ if (def) {
+ if (virNetworkDefSetMetadata(def, type, metadata, key, uri) < 0)
+ return -1;
+
+ if (virNetworkObjSaveStatus(stateDir, net, xmlopt) < 0)
+ return -1;
+ }
+
+ if (persistentDef) {
+ if (virNetworkDefSetMetadata(persistentDef, type, metadata, key,
+ uri) < 0)
+ return -1;
+
+ if (virNetworkSaveConfig(configDir, persistentDef, xmlopt) < 0)
+ return -1;
+ }
+
+ return 0;
+}
diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h
index 7d34fa3204..a4ed28b1bc 100644
--- a/src/conf/virnetworkobj.h
+++ b/src/conf/virnetworkobj.h
@@ -258,3 +258,20 @@ virNetworkObjListNumOfNetworks(virNetworkObjList *nets,
void
virNetworkObjListPrune(virNetworkObjList *nets,
unsigned int flags);
+
+char *
+virNetworkObjGetMetadata(virNetworkObj *network,
+ int type,
+ const char *uri,
+ unsigned int flags);
+
+int
+virNetworkObjSetMetadata(virNetworkObj *network,
+ int type,
+ const char *metadata,
+ const char *key,
+ const char *uri,
+ virNetworkXMLOption *xmlopt,
+ const char *stateDir,
+ const char *configDir,
+ unsigned int flags);
--
2.41.0On 7/11/23 08:47, K Shiva Kiran wrote:
> - Introduces virNetworkObjGetMetadata() and
> virNetworkObjSetMetadata().
> - These functions implement common behaviour that can be reused by
> network drivers that use the virNetworkObj struct.
> - Also adds helper functions that resolve the live/persistent state of
> the network before setting metadata.
>
> Signed-off-by: K Shiva Kiran <shiva_kr@riseup.net>
> ---
> src/conf/virnetworkobj.c | 325 +++++++++++++++++++++++++++++++++++++++
> src/conf/virnetworkobj.h | 17 ++
> 2 files changed, 342 insertions(+)
>
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index b8b86da06f..41d0a1d971 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -1822,3 +1822,328 @@ virNetworkObjLoadAllPorts(virNetworkObj *net,
>
> return 0;
> }
> +
> +
> +/**
> + * virNetworkObjUpdateModificationImpact:
> + *
> + * @net: network object
> + * @flags: flags to update the modification impact on
> + *
> + * Resolves virNetworkUpdateFlags in @flags so that they correctly
> + * apply to the actual state of @net. @flags may be modified after call to this
> + * function.
> + *
> + * Returns 0 on success if @flags point to a valid combination for @net or -1 on
> + * error.
> + */
> +static int
> +virNetworkObjUpdateModificationImpact(virNetworkObj *net,
> + unsigned int *flags)
> +{
> + bool isActive = virNetworkObjIsActive(net);
> +
> + if ((*flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE | VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
> + VIR_NETWORK_UPDATE_AFFECT_CURRENT) {
> + if (isActive)
> + *flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE;
> + else
> + *flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG;
> + }
> +
> + if (!isActive && (*flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("network is not running"));
> + return -1;
> + }
> +
> + if (!net->persistent && (*flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG)) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("transient networks do not have any "
> + "persistent config"));
I'm going to mention it here, but it applies to the all of your patches,
and all the new code in fact. We made an exemption for error messages
and thus they don't need to be broken at 80 chars limit. In fact, they
shouldn't. The reason is simple: easier grep as one doesn't have to try
and guess how is the message broken into individual substrings. Of
course, you will find plenty of "bad" examples throughout the code, but
that's because it is an old code. Whenever we rewrite something or
introduce new code this exception applies and the old code is, well,
gradually fixed.
> + return -1;
> + }
> +
> + return 0;
This code is basically the same as in networkUpdate(). The first part
that sets _LIVE or _CONFIG is there verbatim, the second one is hidden
under virNetworkObjConfigChangeSetup(). There's one extra step that the
function does - it calls virNetworkObjSetDefTransient() but I don't
think that's necessary. Either the network is active and
virNetworkObjSetDefTransient() was already called, or is inactive and
thus it's a NOP. IOW, the call to virNetworkObjSetDefTransient() can be
removed.
After this, virNetworkObjUpdateModificationImpact() could be exported
(just like corresponding virDomain* sibling function is) so that it can
be called from both src/conf/virnetworkobj.c and
src/network/bridge_driver.c. Because I think we can get away with the
networkUpdate() doing the following:
networkUpdate() {
...
virNetworkObjUpdateModificationImpact();
if (isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
/* This is the check that possibly calls
networkRemoveFirewallRules() and sets needFirewallRefresh = true */
}
virNetworkObjUpdate();
...
}
BTW: when cooking the patch, don't forget the same pattern is copied to
our test driver (src/test/test_driver.c).
Michal
On 19/07/23 14:48, Michal Prívozník wrote:
> On 7/11/23 08:47, K Shiva Kiran wrote:
>> - Introduces virNetworkObjGetMetadata() and
>> virNetworkObjSetMetadata().
>> - These functions implement common behaviour that can be reused by
>> network drivers that use the virNetworkObj struct.
>> - Also adds helper functions that resolve the live/persistent state of
>> the network before setting metadata.
>>
>> Signed-off-by: K Shiva Kiran<shiva_kr@riseup.net>
>> ---
>> src/conf/virnetworkobj.c | 325 +++++++++++++++++++++++++++++++++++++++
>> src/conf/virnetworkobj.h | 17 ++
>> 2 files changed, 342 insertions(+)
>>
>> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
>> index b8b86da06f..41d0a1d971 100644
>> --- a/src/conf/virnetworkobj.c
>> +++ b/src/conf/virnetworkobj.c
>> @@ -1822,3 +1822,328 @@ virNetworkObjLoadAllPorts(virNetworkObj *net,
>>
>> return 0;
>> }
>> +
>> +
>> +/**
>> + * virNetworkObjUpdateModificationImpact:
>> + *
>> + * @net: network object
>> + * @flags: flags to update the modification impact on
>> + *
>> + * Resolves virNetworkUpdateFlags in @flags so that they correctly
>> + * apply to the actual state of @net. @flags may be modified after call to this
>> + * function.
>> + *
>> + * Returns 0 on success if @flags point to a valid combination for @net or -1 on
>> + * error.
>> + */
>> +static int
>> +virNetworkObjUpdateModificationImpact(virNetworkObj *net,
>> + unsigned int *flags)
>> +{
>> + bool isActive = virNetworkObjIsActive(net);
>> +
>> + if ((*flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE | VIR_NETWORK_UPDATE_AFFECT_CONFIG)) ==
>> + VIR_NETWORK_UPDATE_AFFECT_CURRENT) {
>> + if (isActive)
>> + *flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE;
>> + else
>> + *flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG;
>> + }
>> +
>> + if (!isActive && (*flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
>> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> + _("network is not running"));
>> + return -1;
>> + }
>> +
>> + if (!net->persistent && (*flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG)) {
>> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> + _("transient networks do not have any "
>> + "persistent config"));
> I'm going to mention it here, but it applies to the all of your patches,
> and all the new code in fact. We made an exemption for error messages
> and thus they don't need to be broken at 80 chars limit. In fact, they
> shouldn't. The reason is simple: easier grep as one doesn't have to try
> and guess how is the message broken into individual substrings. Of
> course, you will find plenty of "bad" examples throughout the code, but
> that's because it is an old code. Whenever we rewrite something or
> introduce new code this exception applies and the old code is, well,
> gradually fixed.
>
>> + return -1;
>> + }
>> +
>> + return 0;
> This code is basically the same as in networkUpdate(). The first part
> that sets _LIVE or _CONFIG is there verbatim, the second one is hidden
> under virNetworkObjConfigChangeSetup(). There's one extra step that the
> function does - it calls virNetworkObjSetDefTransient() but I don't
> think that's necessary. Either the network is active and
> virNetworkObjSetDefTransient() was already called, or is inactive and
> thus it's a NOP. IOW, the call to virNetworkObjSetDefTransient() can be
> removed.
>
> After this, virNetworkObjUpdateModificationImpact() could be exported
> (just like corresponding virDomain* sibling function is) so that it can
> be called from both src/conf/virnetworkobj.c and
> src/network/bridge_driver.c. Because I think we can get away with the
> networkUpdate() doing the following:
>
> networkUpdate() {
> ...
> virNetworkObjUpdateModificationImpact();
>
> if (isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) {
> /* This is the check that possibly calls
> networkRemoveFirewallRules() and sets needFirewallRefresh = true */
> }
>
> virNetworkObjUpdate();
> ...
> }
>
> BTW: when cooking the patch, don't forget the same pattern is copied to
> our test driver (src/test/test_driver.c).
>
>
> Michal
>
Applied in v2:
https://listman.redhat.com/archives/libvir-list/2023-July/240831.html
I have forgotten to mention the diff w.r.t v1 within the v2 patch itself,
thus have included it in a reply.
Shiva
© 2016 - 2026 Red Hat, Inc.