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;