On 6/26/23 15:55, K Shiva wrote:
> - The definition of struct virNetworkObj has been moved from
> virnetworkobj.c to its header as it was needed by network_event.h.
> - Added functions to parse and save the XML along with helper functions
> that resolve the live/persistent state of the network.
>
> Signed-off-by: K Shiva <shiva_kr@xxxxxxxxxx>
> ---
> src/conf/network_conf.c | 3 +
> src/conf/network_conf.h | 2 +
> src/conf/virnetworkobj.c | 347 ++++++++++++++++++++++++++++++++++++---
> src/conf/virnetworkobj.h | 56 +++++++
> 4 files changed, 386 insertions(+), 22 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 73788b6d87..84952db041 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2546,6 +2546,9 @@ virNetworkSaveXML(const char *configDir,
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> g_autofree char *configFile = NULL;
>
> + if (!configDir)
> + return 0;
> +
> if ((configFile = virNetworkConfigFile(configDir, def->name)) == NULL)
> return -1;
>
This seems orthogonal to the rest of the changes. I'm not sure why it's
needed but definitely does not belong into this patch.
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 2b2e9d15f0..5a1bdb1284 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -249,6 +249,8 @@ struct _virNetworkDef {
> unsigned char uuid[VIR_UUID_BUFLEN];
> bool uuid_specified;
> char *name;
> + char *title;
> + char *description;
> int connections; /* # of guest interfaces connected to this network */
>
> char *bridge; /* Name of bridge device */
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index b8b86da06f..82f90937bc 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -39,28 +39,6 @@ VIR_LOG_INIT("conf.virnetworkobj");
> * that big. */
> #define INIT_CLASS_ID_BITMAP_SIZE (1<<4)
>
> -struct _virNetworkObj {
> - virObjectLockable parent;
> -
> - pid_t dnsmasqPid;
> - bool active;
> - bool autostart;
> - bool persistent;
> -
> - virNetworkDef *def; /* The current definition */
> - virNetworkDef *newDef; /* New definition to activate at shutdown */
> -
> - virBitmap *classIdMap; /* bitmap of class IDs for QoS */
> - unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */
> -
> - unsigned int taint;
> -
> - /* Immutable pointer, self locking APIs */
> - virMacMap *macmap;
> -
> - GHashTable *ports; /* uuid -> virNetworkPortDef **/
> -};
> -
> struct _virNetworkObjList {
> virObjectRWLockable parent;
>
> @@ -1822,3 +1800,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.
> + */
> +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.
> + */
> +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.
> + */
> +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.
> + */
> +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..d17a43d7bb 100644
> --- a/src/conf/virnetworkobj.h
> +++ b/src/conf/virnetworkobj.h
> @@ -26,6 +26,28 @@
>
> typedef struct _virNetworkObj virNetworkObj;
>
> +struct _virNetworkObj {
> + virObjectLockable parent;
> +
> + pid_t dnsmasqPid;
> + bool active;
> + bool autostart;
> + bool persistent;
> +
> + virNetworkDef *def; /* The current definition */
> + virNetworkDef *newDef; /* New definition to activate at shutdown */
> +
> + virBitmap *classIdMap; /* bitmap of class IDs for QoS */
> + unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */
> +
> + unsigned int taint;
> +
> + /* Immutable pointer, self locking APIs */
> + virMacMap *macmap;
> +
> + GHashTable *ports; /* uuid -> virNetworkPortDef **/
> +};
> +
No, the struct was hidden for a reason: we don't want the rest of the
code to fiddle with struct members directly but use so called getter and
setter functions (functions which get/set value of individual struct
members), e.g. virNetworkObjIsActive() / virNetworkObjSetActive(). This
way, code is more auditable, i.e. it's easy to see where a given struct
member is set. Not only that, but individual getters/setters might
perform additional tasks tied to the struct member, e.g. lock the object.
> virNetworkObj *
> virNetworkObjNew(void);
>
> @@ -258,3 +280,37 @@ virNetworkObjListNumOfNetworks(virNetworkObjList *nets,
> void
> virNetworkObjListPrune(virNetworkObjList *nets,
> unsigned int flags);
> +
> +int virNetworkObjUpdateModificationImpact(virNetworkObj *net,
> + unsigned int *flags);
> +
> +int
> +virNetworkObjGetDefs(virNetworkObj *net,
> + unsigned int flags,
> + virNetworkDef **liveDef,
> + virNetworkDef **persDef);
> +
> +virNetworkDef *
> +virNetworkObjGetOneDefState(virNetworkObj *net,
> + unsigned int flags,
> + bool *state);
> +virNetworkDef *
> +virNetworkObjGetOneDef(virNetworkObj *net,
> + unsigned int flags);
Neither of these ^^^ functions is called from outside of
virnetworkobj.c. What's the reason for their exposure?
> +
> +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);
BTW: this is where I stop my review, as these patches need to be split
differently.
Michal
