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@xxxxxxxxx>
> ---
> 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;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list