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

Re: [libvirt] [RFC PATCH 2/5] conf: Parse and format ivshmem device XML



On 2014/8/6 0:48, Maxime Leroy wrote:
> This patch adds configuration support for the ivshmem device
> as described in the schema in the previous patch.
> 
> Signed-off-by: Maxime Leroy <maxime leroy 6wind com>
> ---
>  src/conf/domain_conf.c   | 234 ++++++++++++++++++++++++++++++++++++++++++++++-
>  src/conf/domain_conf.h   |  40 ++++++++
>  src/libvirt_private.syms |   4 +
>  3 files changed, 277 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c25c74b..829f1bf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c

[...]
>  
>      /* This switch statement is here to trigger compiler warning when adding
>       * a new device type. When you are adding a new field to the switch you
> @@ -2805,6 +2841,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
>      case VIR_DOMAIN_DEVICE_NVRAM:
>      case VIR_DOMAIN_DEVICE_LAST:
>      case VIR_DOMAIN_DEVICE_RNG:
> +    case VIR_DOMAIN_DEVICE_IVSHMEM:
>          break;

The function is good in logic. But I think it's better to keep VIR_DOMAIN_DEVICE_LAST
the last case. (Also case VIR_DOMAIN_DEVICE_RNG should be moved ahead of
case VIR_DOMAIN_DEVICE_LAST)

>      }
>  
> @@ -9354,6 +9391,124 @@ virDomainNVRAMDefParseXML(xmlNodePtr node,
>      return NULL;
>  }
>  
> +static virDomainIvshmemDefPtr
> +virDomainIvshmemDefParseXML(xmlNodePtr node,
> +                            xmlXPathContextPtr ctxt,
> +                            unsigned int flags)
> +{
> +    virDomainIvshmemDefPtr def;
> +    char *use_server = NULL;
> +    char *role = NULL;
> +    char *ioeventfd = NULL;
> +    char *vectors = NULL;
> +    xmlNodePtr cur;
> +    xmlNodePtr save = ctxt->node;
> +
> +    if (VIR_ALLOC(def) < 0)
> +        return NULL;
> +
> +    use_server = virXMLPropString(node, "use_server");
> +    if (use_server !=  NULL) {
> +        if ((def->use_server
> +             = virDomainIvshmemServerTypeFromString(use_server)) < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown ivshmem use_server tyoe '%s'"), use_server);
s/tyoe/type

> +            goto error;
> +        }
> +    } else {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       "%s", _("missing use_server type"));
> +        goto error;
> +    }
> +

[...]
> @@ -10210,6 +10365,10 @@ virDomainDeviceDefParse(const char *xmlStr,
>          if (!(dev->data.nvram = virDomainNVRAMDefParseXML(node, flags)))
>              goto error;
>          break;
> +    case VIR_DOMAIN_DEVICE_IVSHMEM:
> +        if (!(dev->data.ivshmem = virDomainIvshmemDefParseXML(node, ctxt, flags)))
> +            goto error;
> +        break;
>      case VIR_DOMAIN_DEVICE_NONE:
>      case VIR_DOMAIN_DEVICE_LAST:
>          break;
> @@ -13102,6 +13261,25 @@ virDomainDefParseXML(xmlDocPtr xml,
>          VIR_FREE(nodes);
>      }
>  
> +    /* analysis of the ivshmem devices */
> +    if ((n = virXPathNodeSet("./devices/ivshmem", ctxt, &nodes)) < 0) {
> +        goto error;
> +    }
> +    if (n && VIR_ALLOC_N(def->ivshmems, n) < 0)
> +        goto error;
> +
> +    node = ctxt->node;
> +    for (i = 0; i < n; i++) {
> +        virDomainIvshmemDefPtr ivshmem;
> +        ctxt->node = nodes[i];
> +        ivshmem = virDomainIvshmemDefParseXML(nodes[i], ctxt, flags);
> +        if (!ivshmem)
> +            goto error;
> +
> +        def->ivshmems[def->nivshmems++] = ivshmem;
> +    }
> +    ctxt->node = node;
> +    VIR_FREE(nodes);
> 
Here actions: node = ctxt->node; ctxt->node = nodes[i]; ctxt->node = node;
I see other devices' xml parsing function virDomainXXXParseXML (such as virDomainRNGDefParseXML).
These actions are in the function virDomainXXXparesXML().
So are the actions here are redundant? Or should be moved into virDomainIvshmemDefParseXML.

>      /* analysis of the user namespace mapping */
>      if ((n = virXPathNodeSet("./idmap/uid", ctxt, &nodes)) < 0)
> @@ -16701,6 +16879,55 @@ static int virDomainPanicDefFormat(virBufferPtr buf,
>      return 0;
>  }
>  
> +static int virDomainIvshmemDefFormat(virBufferPtr buf,
> +                                     virDomainIvshmemDefPtr def,
> +                                     unsigned int flags)
> +{
> +    const char *use_server = virDomainIvshmemServerTypeToString(def->use_server);
> +    const char *role = virDomainIvshmemRoleTypeToString(def->role);
> +
> +    if (!use_server) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unexpected ivshmem server %d"), def->use_server);
> +        return -1;
> +    }
> +
> +    if (!role) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unexpected ivshmem role %d"), def->role);
> +        return -1;
> +    }
> +
> +    virBufferAsprintf(buf, "<ivshmem use_server='%s'", use_server);
> +    if (def->role)
> +        virBufferAsprintf(buf, " role='%s'", role);
> +    virBufferAddLit(buf, ">\n");

def->role is type of int(ENUM). Do you want to check def->role is zero?
In other words, is role='default' illegal ?

> +
> +    virBufferAdjustIndent(buf, 2);
> +    virBufferAsprintf(buf, "<source file='%s'/>\n", def->file);
> +    if (def->size)
> +        virBufferAsprintf(buf, "<size unit='M'>%llu</size>\n",
> +                          def->size  / (1024 * 1024));
> +
> +    if (def->use_server == VIR_DOMAIN_IVSHMEM_SERVER_ENABLED && def->msi.enabled) {
> +            virBufferAddLit(buf, "<msi");
> +            if (def->msi.vectors)
> +                virBufferAsprintf(buf, " vectors='%u'", def->msi.vectors);
> +            if (def->msi.ioeventfd)
> +                virBufferAsprintf(buf, " ioeventfd='%s'",
> +                                  virTristateSwitchTypeToString(def->msi.ioeventfd));
> +            virBufferAddLit(buf, "/>\n");
> +    }
> +
> +    if (virDomainDeviceInfoIsSet(&def->info, flags) &&
> +        virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
> +        return -1;
> +    virBufferAdjustIndent(buf, -2);
> +    virBufferAddLit(buf, "</ivshmem>\n");
> +
> +    return 0;
> +}
> +
>  static int
>  virDomainRNGDefFormat(virBufferPtr buf,
>                        virDomainRNGDefPtr def,
> @@ -18250,6 +18477,10 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>          virDomainPanicDefFormat(buf, def->panic) < 0)
>          goto error;
>  
> +    for (n = 0; n < def->nivshmems; n++)
> +        if (virDomainIvshmemDefFormat(buf, def->ivshmems[n], flags) < 0)
> +            goto error;
> +
>      virBufferAdjustIndent(buf, -2);
>      virBufferAddLit(buf, "</devices>\n");
>  
> @@ -19615,6 +19846,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
>      case VIR_DOMAIN_DEVICE_SMARTCARD:
>      case VIR_DOMAIN_DEVICE_MEMBALLOON:
>      case VIR_DOMAIN_DEVICE_NVRAM:
> +    case VIR_DOMAIN_DEVICE_IVSHMEM:
>      case VIR_DOMAIN_DEVICE_LAST:
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Copying definition of '%d' type "
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index bffc0a5..af499b4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -136,6 +136,9 @@ typedef virDomainPanicDef *virDomainPanicDefPtr;
>  typedef struct _virDomainChrSourceDef virDomainChrSourceDef;
>  typedef virDomainChrSourceDef *virDomainChrSourceDefPtr;
>  
> +typedef struct _virDomainIvshmemDef virDomainIvshmemDef;
> +typedef virDomainIvshmemDef *virDomainIvshmemDefPtr;
> +
>  /* Flags for the 'type' field in virDomainDeviceDef */
>  typedef enum {
>      VIR_DOMAIN_DEVICE_NONE = 0,
> @@ -157,6 +160,7 @@ typedef enum {
>      VIR_DOMAIN_DEVICE_MEMBALLOON,
>      VIR_DOMAIN_DEVICE_NVRAM,
>      VIR_DOMAIN_DEVICE_RNG,
> +    VIR_DOMAIN_DEVICE_IVSHMEM,
>  
>      VIR_DOMAIN_DEVICE_LAST
>  } virDomainDeviceType;
> @@ -184,6 +188,7 @@ struct _virDomainDeviceDef {
>          virDomainMemballoonDefPtr memballoon;
>          virDomainNVRAMDefPtr nvram;
>          virDomainRNGDefPtr rng;
> +        virDomainIvshmemDefPtr ivshmem;
>      } data;
>  };
>  
> @@ -598,6 +603,22 @@ typedef enum {
>      VIR_DOMAIN_DISK_DISCARD_LAST
>  } virDomainDiskDiscard;
>  
> +typedef enum {
> +    VIR_DOMAIN_IVSHMEM_SERVER_ENABLED = 0,
> +    VIR_DOMAIN_IVSHMEM_SERVER_DISABLED,
> +
> +    VIR_DOMAIN_IVSHMEM_SERVER_LAST,
> +} virDomainIvshmemServer;
> +
> +
> +typedef enum {
> +    VIR_DOMAIN_IVSHMEM_ROLE_DEFAULT = 0,
> +    VIR_DOMAIN_IVSHMEM_ROLE_MASTER,
> +    VIR_DOMAIN_IVSHMEM_ROLE_PEER,
> +
> +    VIR_DOMAIN_IVSHMEM_ROLE_LAST,
> +} virDomainIvshmemRole;
> +
>  typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo;
>  struct _virDomainBlockIoTuneInfo {
>      unsigned long long total_bytes_sec;
> @@ -1485,6 +1506,19 @@ struct _virDomainNVRAMDef {
>      virDomainDeviceInfo info;
>  };
>  
> +struct _virDomainIvshmemDef {
> +    int use_server; /* enum virDomainIvshmemServer */
> +    int role; /* virDomainIvshmemRole */
> +    unsigned long long size;
> +    char *file;
> +    struct {
> +        bool enabled;
> +        unsigned vectors;
> +        virTristateSwitch ioeventfd;
> +    } msi;
> +    virDomainDeviceInfo info;
> +};
> +
>  typedef enum {
>      VIR_DOMAIN_SMBIOS_NONE = 0,
>      VIR_DOMAIN_SMBIOS_EMULATE,
> @@ -2006,6 +2040,9 @@ struct _virDomainDef {
>      size_t nrngs;
>      virDomainRNGDefPtr *rngs;
>  
> +    size_t nivshmems;
> +    virDomainIvshmemDefPtr *ivshmems;
> +
>      /* Only 1 */
>      virDomainWatchdogDefPtr watchdog;
>      virDomainMemballoonDefPtr memballoon;
> @@ -2203,6 +2240,7 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def);
>  void virDomainHubDefFree(virDomainHubDefPtr def);
>  void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def);
>  void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def);
> +void virDomainIvshmemDefFree(virDomainIvshmemDefPtr def);
>  void virDomainDeviceDefFree(virDomainDeviceDefPtr def);
>  virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
>                                               const virDomainDef *def,
> @@ -2629,6 +2667,8 @@ VIR_ENUM_DECL(virDomainRNGModel)
>  VIR_ENUM_DECL(virDomainRNGBackend)
>  VIR_ENUM_DECL(virDomainTPMModel)
>  VIR_ENUM_DECL(virDomainTPMBackend)
> +VIR_ENUM_DECL(virDomainIvshmemServer)
> +VIR_ENUM_DECL(virDomainIvshmemRole)
>  /* from libvirt.h */
>  VIR_ENUM_DECL(virDomainState)
>  VIR_ENUM_DECL(virDomainNostateReason)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 08111d4..794f3dd 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -300,6 +300,10 @@ virDomainHubTypeToString;
>  virDomainHypervTypeFromString;
>  virDomainHypervTypeToString;
>  virDomainInputDefFree;
> +virDomainIvshmemRoleTypeFromString;
> +virDomainIvshmemRoleTypeToString;
> +virDomainIvshmemServerTypeFromString;
> +virDomainIvshmemServerTypeToString;
>  virDomainLeaseDefFree;
>  virDomainLeaseIndex;
>  virDomainLeaseInsert;



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