Re: [libvirt] [PATCH v2 4/8] Introduce PCI Multifunction device parser

On 05/18/2016 05:31 PM, Shivaprasad G Bhat wrote: > This patch just introduces the parser function used by > the later patches. The parser disallows hostdevices to be > used with other virtio devices simultaneously. > Why? (and I think you mean "emulated" not "virtio")

> Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com&gt; > --- > src/conf/domain_conf.c | 236 > ++++++++++++++++++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 22 ++++ > src/libvirt_private.syms | 3 + > 3 files changed, 261 insertions(+) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index ed0c471..e946147 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -860,6 +860,36 @@ virDomainXMLOptionClassDispose(void *obj) > (xmlopt->config.privFree)(xmlopt->config.priv); > } > +/* virDomainDeviceDefListAddCopy - add a *copy* of the device to this > list */ > +int > +virDomainDeviceDefListAddCopy(virDomainDeviceDefListPtr list, > + virDomainDeviceDefPtr dev, > + const virDomainDef *def, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt) > +{ > + virDomainDeviceDefPtr copy = virDomainDeviceDefCopy(dev, def, caps, > xmlopt); > + > + if (!copy) > + return -1; > + if (VIR_APPEND_ELEMENT(list->devs, list->count, copy) < 0) { > + virDomainDeviceDefFree(copy); > + return -1; > + } > + return 0; > +} > + > +void virDomainDeviceDefListDispose(virDomainDeviceDefListPtr list) > +{ > + size_t i; > + > + if (!list) > + return; > + for (i = 0; i < list->count; i++) > + virDomainDeviceDefFree(list->devs[i]); > + VIR_FREE(list); > +} > This isn't a vir*Dispose() function, it is a vir*Free() function. the Dispose() functions are used for virObject-based objects, and take a void *obj as their argument.

+ > /** > * virDomainKeyWrapCipherDefParseXML: > * > @@ -24365,3 +24395,209 @@ virDomainObjGetShortName(virDomainObjPtr vm) > return ret; > } > + > +static int > +virDomainPCIMultifunctionDeviceDefParseXML(xmlXPathContextPtr ctxt, > + const virDomainDef *def, > + virDomainDeviceDefListPtr > devlist, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > Instead of all these loops for each type of device. I think it would make more sense to separate virDomainDeviceDefParse() so that the original function was: virDomainDeviceDefParse(....) { ... if (!(xml = virXMLParseString(......))) return -1; node = ctxt->node; dev = virDomainDeviceDefParseXML(node, ctxt, def, caps, xmlopt, flags))) xmlFreeDoc(xml) xmlXPathFreeContext(ctxt); return dev; } with a new function virDomainDeviceDefParseXML() that contained all the rest of the insides of the original function. You could then call this new function for each node of the xmlDocPtr that you get after parsing the input to your new "parse multiple devices" function (which could maybe be called virDomainDeviceDefParseMany()). After all of the devices are parsed, then you can validate that they are all PCI devices, and each for a different function of the same slot (if an address has been assigned).

+ size_t i, j; > + int n; > + virDomainDeviceDef device; > + xmlNodePtr *nodes = NULL; > + char *netprefix = NULL; > + int nhostdevs = 0; > + > + device.type = VIR_DOMAIN_DEVICE_DISK; > + > + if ((n = virXPathNodeSet("./disk", ctxt, &nodes)) < 0) > + goto error; > + > + for (i = 0; i < n; i++) { > + virDomainDiskDefPtr disk = virDomainDiskDefParseXML(xmlopt, > + nodes[i], > + ctxt, > + NULL, > + > def->seclabels, > + > def->nseclabels, > + flags); > + if (!disk) > + goto error; > + > + device.data.disk = disk; > + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, > xmlopt) < 0) > + goto error; > + VIR_FREE(disk); > + } > + VIR_FREE(nodes); > + > + device.type = VIR_DOMAIN_DEVICE_CONTROLLER; > + /* analysis of the controller devices */ > + if ((n = virXPathNodeSet("./controller", ctxt, &nodes)) < 0) > + goto error; > + > + for (i = 0; i < n; i++) { > + virDomainControllerDefPtr controller = > virDomainControllerDefParseXML(nodes[i], > + > ctxt, > + > flags); > + if (!controller) > + goto error; > + > + device.data.controller = controller; > + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, > xmlopt) < 0) > + goto error; > + VIR_FREE(controller); > + } > + VIR_FREE(nodes); > + > + device.type = VIR_DOMAIN_DEVICE_NET; > + if ((n = virXPathNodeSet("./interface", ctxt, &nodes)) < 0) > + goto error; > + > + netprefix = caps->host.netprefix; > + for (i = 0; i < n; i++) { > + virDomainNetDefPtr net = virDomainNetDefParseXML(xmlopt, > + nodes[i], > + ctxt, > + NULL, > + netprefix, > + flags); > + if (!net) > + goto error; > + > + device.data.net = net; > + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, > xmlopt) < 0) > + goto error; > + VIR_FREE(net); > + } > + VIR_FREE(nodes); > + > + /* analysis of the host devices */ > + device.type = VIR_DOMAIN_DEVICE_HOSTDEV; > + if ((nhostdevs = virXPathNodeSet("./hostdev", ctxt, &nodes)) < 0) > + goto error; > + if (nhostdevs && devlist->count) > + goto misconfig; > + for (i = 0; i < nhostdevs; i++) { > + virDomainHostdevDefPtr hostdev; > + > + hostdev = virDomainHostdevDefParseXML(xmlopt, nodes[i], ctxt, > + NULL, flags); > + if (!hostdev) > + goto error; > + > + if (hostdev->source.subsys.type == > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB || > + hostdev->source.subsys.type == > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Can't add host USB device: " > + "USB is disabled in this host")); > + virDomainHostdevDefFree(hostdev); > + goto error; > + } > + device.data.hostdev = hostdev; > + for (j = 0; j < devlist->count; j++) { > + if (virDomainHostdevMatch(hostdev, > devlist->devs[j]->data.hostdev)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Duplicate host devices in list")); > + goto error; > + } > + } > + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, > xmlopt) < 0) > + goto error; > + VIR_FREE(hostdev); > + } > + VIR_FREE(nodes); > + > + /* Parse the RNG devices */ > + device.type = VIR_DOMAIN_DEVICE_RNG; > + if ((n = virXPathNodeSet("./rng", ctxt, &nodes)) < 0) > + goto error; > + for (i = 0; i < n; i++) { > + virDomainRNGDefPtr rng = virDomainRNGDefParseXML(nodes[i], > + ctxt, > + flags); > + if (!rng) > + goto error; > + > + device.data.rng = rng; > + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, > xmlopt) < 0) > + goto error; > + VIR_FREE(rng); > + } > + VIR_FREE(nodes); > + > + device.type = VIR_DOMAIN_DEVICE_CHR; > + if ((n = virXPathNodeSet("./serial", ctxt, &nodes)) < 0) > + goto error; > + > + for (i = 0; i < n; i++) { > + virDomainChrDefPtr chr = virDomainChrDefParseXML(ctxt, > + nodes[i], > + def->seclabels, > + def->nseclabels, > + flags); > + if (!chr) > + goto error; > + > + if (chr->target.port == -1) { > + int maxport = -1; > + for (j = 0; j < i; j++) { > + if (def->serials[j]->target.port > maxport) > + maxport = def->serials[j]->target.port; > + } > + chr->target.port = maxport + 1; > + } > + device.data.chr = chr; > + if (virDomainDeviceDefListAddCopy(devlist, &device, def, caps, > xmlopt) < 0) > + goto error; > + VIR_FREE(chr); > + } > + VIR_FREE(nodes); > + > + if (nhostdevs && nhostdevs != devlist->count) > + goto misconfig; > + > + return 0; > + misconfig: > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Shouldn't mix host devices with other devices")); > + error: > + return -1; > +} > + > + > +int > +virDomainPCIMultifunctionDeviceDefParseNode(const char *xml, > + const virDomainDef *def, > + virDomainDeviceDefListPtr devlist, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > + xmlXPathContextPtr ctxt = NULL; > + int ret = -1; > + > + xmlDocPtr xmlptr; > + > + if (!(xmlptr = virXMLParse(NULL, xml, _("(device_definition)")))) { > + /* We report error anyway later */ > + return -1; > + } > + > + ctxt = xmlXPathNewContext(xmlptr); > + if (ctxt == NULL) { > + virReportOOMError(); > + goto cleanup; > + } > + > + ctxt->node = xmlDocGetRootElement(xmlptr); > + ret = virDomainPCIMultifunctionDeviceDefParseXML(ctxt, def, devlist, > + caps, xmlopt, > flags); > + > + cleanup: > + xmlXPathFreeContext(ctxt); > + return ret; > +} > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index b9e696d..9ddfbd1 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -2462,6 +2462,20 @@ typedef enum { > typedef struct _virDomainXMLOption virDomainXMLOption; > typedef virDomainXMLOption *virDomainXMLOptionPtr; > +struct virDomainDeviceDefList { > + virDomainDeviceDefPtr *devs; > + size_t count; > +}; > +typedef struct virDomainDeviceDefList *virDomainDeviceDefListPtr; > + > +int > +virDomainDeviceDefListAddCopy(virDomainDeviceDefListPtr list, > virDomainDeviceDefPtr dev, > + const virDomainDef *def, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt); > +void virDomainDeviceDefListDispose(virDomainDeviceDefListPtr list); > + > + > /* Called once after everything else has been parsed, for adjusting > * overall domain defaults. */ > typedef int (*virDomainDefPostParseCallback)(virDomainDefPtr def, > @@ -3176,6 +3190,14 @@ int > virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def, > bool virDomainDefHasMemballoon(const virDomainDef *def) > ATTRIBUTE_NONNULL(1); > +int > +virDomainPCIMultifunctionDeviceDefParseNode(const char *xml, > + const virDomainDef *def, > + virDomainDeviceDefListPtr devlist, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags); > + > char *virDomainObjGetShortName(virDomainObjPtr vm); > #endif /* __DOMAIN_CONF_H */ > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index e4953b7..d6a60b5 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -109,6 +109,7 @@ virDomainPCIAddressSetGrow; > virDomainPCIAddressSlotInUse; > virDomainPCIAddressValidate; > virDomainPCIControllerModelToConnectType; > +virDomainPCIMultifunctionDeviceDefParseNode; > virDomainVirtioSerialAddrAssign; > virDomainVirtioSerialAddrAutoAssign; > virDomainVirtioSerialAddrIsComplete; > @@ -249,6 +250,8 @@ virDomainDeviceAddressIsValid; > virDomainDeviceAddressTypeToString; > virDomainDeviceDefCopy; > virDomainDeviceDefFree; > +virDomainDeviceDefListAddCopy; > +virDomainDeviceDefListDispose; > virDomainDeviceDefParse; > virDomainDeviceFindControllerModel; > virDomainDeviceGetInfo; > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > >