Re: [libvirt PATCH v3 1/8] Parser and Schema definitions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux