[libvirt] [PATCH 0/6] (informational) conf support for <interface type='hostdev'>
This series of patches enhances the domain parser/formatter code to support a sort of "intelligent hostdev", i.e. PCI passthrough where device-type specific initialization is done prior to assigning the device to the guest, in particular to allow setting the MAC address and do 802.1QbX setup for network devices. Rather than adding all of the device-type specific config to <hostdev>, I accomplish this through adding a new type of <interface> element, type='hostdev'. When an interface is type='hostdev' the following is changed: * in the toplevel device, the managed attribute can be specified (with identical results as when it's specified in a <hostdev> * The <source> element can specify a pci address or usb address, just as can be done in <hostdev>. One notable difference is that the type of the address is specified directly in the source <address> element, rather than as an attribute of the toplevel device (that's how it's done for <hostdev>, but for <interface>, the toplevel element's type attribute is already used). These patches include only the parser/formatter, test (for domainschematest, and qemuxml2xmltest only), and doc changes; they still need to be hooked into the domain's device lists (a type=hostdev interface will reside in both the interface list (for configuration and memory management) and hostdev list (for attach/detach, and tracking of which devices are assigned)). I don't intend to push these patches until that part is also done, but some other people are doing work that depends on the changes to config format and data structures, so I'm sending these patches now both to unblock them, as well as to get feedback from everyone else on what needs to be changed. The first 4 patches are just setup for the real new code - they reorder and refactor existing code to allow greater re-use of existing code and easier plugin of the new code. Patch 5/6 is just a couple of lines, and patch 6/6 provides the new functionality.
In order to allow for a virDomainHostdevDef that uses the virDomainDeviceInfo of a "higher level" device (such as a virDomainNetDef), this patch changes the virDomainDeviceInfo in the HostdevDef into a virDomainDeviceInfoPtr. Rather than adding checks all over the code to check for a null info, we just guarantee that it is always valid. The new function virDomainHostdevDefAlloc() allocates a virDomainDeviceInfo and plugs it in, and virDomainHostdevDefFree() makes sure it is freed. There were 4 places allocating virDomainHostdevDefs, all of them parsers of one sort or another, and those have all had their VIR_ALLOC(hostdev) changed to virDomainHostdevDefAlloc(). Other than that, and the new functions, all the rest of the changes are just mechanical removals of "&" or changing "." to "->". --- src/conf/domain_conf.c | 48 +++++++++++++++++++++++++++++++++------------ src/conf/domain_conf.h | 3 +- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 45 +++++++++++++++++++----------------------- src/qemu/qemu_hotplug.c | 28 +++++++++++++------------- src/xenxs/xen_sxpr.c | 4 +- src/xenxs/xen_xm.c | 4 +- 7 files changed, 76 insertions(+), 57 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85a2058..137fb73 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -780,6 +780,15 @@ bool virDomainObjTaint(virDomainObjPtr obj, return true; } +static void +virDomainDeviceInfoFree(virDomainDeviceInfoPtr info) +{ + if (info) { + virDomainDeviceInfoClear(info); + VIR_FREE(info); + } +} + static void virDomainGraphicsAuthDefClear(virDomainGraphicsAuthDefPtr def) @@ -1288,12 +1297,27 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def) VIR_FREE(def); } +virDomainHostdevDefPtr virDomainHostdevDefAlloc(void) +{ + virDomainHostdevDefPtr def = NULL; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + return NULL; + } + if (VIR_ALLOC(def->info) < 0) { + VIR_FREE(def); + return NULL; + } + return def; +} + void virDomainHostdevDefFree(virDomainHostdevDefPtr def) { if (!def) return; - virDomainDeviceInfoClear(&def->info); + virDomainDeviceInfoFree(def->info); VIR_FREE(def); } @@ -1857,7 +1881,7 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, if (cb(def, &def->sounds[i]->info, opaque) < 0) return -1; for (i = 0; i < def->nhostdevs ; i++) - if (cb(def, &def->hostdevs[i]->info, opaque) < 0) + if (cb(def, def->hostdevs[i]->info, opaque) < 0) return -1; for (i = 0; i < def->nvideos ; i++) if (cb(def, &def->videos[i]->info, opaque) < 0) @@ -6262,14 +6286,14 @@ virDomainHostdevSubsysPciDefParseXML(const xmlNodePtr node, char *devaddr = virXMLPropString(cur, "devaddr"); if (devaddr && virDomainParseLegacyDeviceAddress(devaddr, - &def->info.addr.pci) < 0) { + &def->info->addr.pci) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse devaddr parameter '%s'"), devaddr); VIR_FREE(devaddr); goto out; } - def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } else if ((flags & VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES) && xmlStrEqual(cur->name, BAD_CAST "origstates")) { virDomainHostdevOrigStatesPtr states = &def->origstates; @@ -6300,10 +6324,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, virDomainHostdevDefPtr def; char *mode, *type = NULL, *managed = NULL; - if (VIR_ALLOC(def) < 0) { - virReportOOMError(); + if (!(def = virDomainHostdevDefAlloc())) return NULL; - } mode = virXMLPropString(node, "mode"); if (mode) { @@ -6366,8 +6388,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, cur = cur->next; } - if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - if (virDomainDeviceInfoParseXML(node, bootMap, &def->info, + if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (virDomainDeviceInfoParseXML(node, bootMap, def->info, flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM) < 0) goto error; @@ -6376,8 +6398,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { switch (def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI host devices must use 'pci' address type")); goto error; @@ -8927,7 +8949,7 @@ static bool virDomainHostdevDefCheckABIStability(virDomainHostdevDefPtr src, } } - if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) + if (!virDomainDeviceInfoCheckABIStability(src->info, dst->info)) goto cleanup; identical = true; @@ -11463,7 +11485,7 @@ virDomainHostdevDefFormat(virBufferPtr buf, virBufferAddLit(buf, " </source>\n"); - if (virDomainDeviceInfoFormat(buf, &def->info, + if (virDomainDeviceInfoFormat(buf, def->info, flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9c8792a..1f7f25a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1128,8 +1128,8 @@ struct _virDomainHostdevDef { int dummy; } caps; } source; - virDomainDeviceInfo info; /* Guest address */ virDomainHostdevOrigStates origstates; + virDomainDeviceInfoPtr info; /* Guest address */ }; enum virDomainRedirdevBus { @@ -1740,6 +1740,7 @@ void virDomainSoundDefFree(virDomainSoundDefPtr def); void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def); void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); void virDomainVideoDefFree(virDomainVideoDefPtr def); +virDomainHostdevDefPtr virDomainHostdevDefAlloc(void); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainHubDefFree(virDomainHubDefPtr def); void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9e3573f..dc55f7b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -350,6 +350,7 @@ virDomainGraphicsSpiceZlibCompressionTypeFromString; virDomainGraphicsSpiceZlibCompressionTypeToString; virDomainGraphicsTypeFromString; virDomainGraphicsTypeToString; +virDomainHostdevDefAlloc; virDomainHostdevDefFree; virDomainHostdevModeTypeToString; virDomainHostdevSubsysTypeToString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5a34504..dffb6d4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -535,7 +535,7 @@ qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev idx = 0; for (i = 0 ; i < def->nhostdevs ; i++) { int thisidx; - if ((thisidx = qemuDomainDeviceAliasIndex(&def->hostdevs[i]->info, "hostdev")) < 0) { + if ((thisidx = qemuDomainDeviceAliasIndex(def->hostdevs[i]->info, "hostdev")) < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to determine device index for hostdev device")); return -1; @@ -545,7 +545,7 @@ qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev } } - if (virAsprintf(&hostdev->info.alias, "hostdev%d", idx) < 0) { + if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx) < 0) { virReportOOMError(); return -1; } @@ -1384,13 +1384,13 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) /* Host PCI devices */ for (i = 0; i < def->nhostdevs ; i++) { - if (def->hostdevs[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + if (def->hostdevs[i]->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; if (def->hostdevs[i]->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || def->hostdevs[i]->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) continue; - if (qemuDomainPCIAddressSetNextAddr(addrs, &def->hostdevs[i]->info) < 0) + if (qemuDomainPCIAddressSetNextAddr(addrs, def->hostdevs[i]->info) < 0) goto error; } @@ -2863,14 +2863,14 @@ qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, const char *configfd, dev->source.subsys.u.pci.bus, dev->source.subsys.u.pci.slot, dev->source.subsys.u.pci.function); - virBufferAsprintf(&buf, ",id=%s", dev->info.alias); + virBufferAsprintf(&buf, ",id=%s", dev->info->alias); if (configfd && *configfd) virBufferAsprintf(&buf, ",configfd=%s", configfd); - if (dev->info.bootIndex) - virBufferAsprintf(&buf, ",bootindex=%d", dev->info.bootIndex); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (dev->info->bootIndex) + virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex); + if (qemuBuildDeviceAddressStr(&buf, dev->info, qemuCaps) < 0) goto error; - if (qemuBuildRomStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildRomStr(&buf, dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -2956,9 +2956,9 @@ qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev, virBufferAsprintf(&buf, "usb-host,hostbus=%d,hostaddr=%d,id=%s", dev->source.subsys.u.usb.bus, dev->source.subsys.u.usb.device, - dev->info.alias); + dev->info->alias); - if (qemuBuildDeviceAddressStr(&buf, &dev->info, qemuCaps) < 0) + if (qemuBuildDeviceAddressStr(&buf, dev->info, qemuCaps) < 0) goto error; if (virBufferError(&buf)) { @@ -5575,7 +5575,7 @@ qemuBuildCommandLine(virConnectPtr conn, virDomainHostdevDefPtr hostdev = def->hostdevs[i]; char *devstr; - if (hostdev->info.bootIndex) { + if (hostdev->info->bootIndex) { if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -6527,15 +6527,17 @@ cleanup: static virDomainHostdevDefPtr qemuParseCommandLinePCI(const char *val) { - virDomainHostdevDefPtr def = NULL; int bus = 0, slot = 0, func = 0; const char *start; char *end; + virDomainHostdevDefPtr def = virDomainHostdevDefAlloc(); + + if (!def) + goto cleanup; if (!STRPREFIX(val, "host=")) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unknown PCI device syntax '%s'"), val); - VIR_FREE(def); goto cleanup; } @@ -6561,11 +6563,6 @@ qemuParseCommandLinePCI(const char *val) goto cleanup; } - if (VIR_ALLOC(def) < 0) { - virReportOOMError(); - goto cleanup; - } - def->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; def->managed = 1; def->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; @@ -6584,11 +6581,14 @@ cleanup: static virDomainHostdevDefPtr qemuParseCommandLineUSB(const char *val) { - virDomainHostdevDefPtr def = NULL; + virDomainHostdevDefPtr def = virDomainHostdevDefAlloc(); int first = 0, second = 0; const char *start; char *end; + if (!def) + goto cleanup; + if (!STRPREFIX(val, "host:")) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unknown USB device syntax '%s'"), val); @@ -6627,11 +6627,6 @@ qemuParseCommandLineUSB(const char *val) } } - if (VIR_ALLOC(def) < 0) { - virReportOOMError(); - goto cleanup; - } - def->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; def->managed = 0; def->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3dd7c0a..b55052a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -917,14 +917,14 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) goto error; - if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &hostdev->info) < 0) + if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, hostdev->info) < 0) goto error; releaseaddr = true; if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_PCI_CONFIGFD)) { configfd = qemuOpenPCIConfig(hostdev); if (configfd >= 0) { if (virAsprintf(&configfd_name, "fd-%s", - hostdev->info.alias) < 0) { + hostdev->info->alias) < 0) { virReportOOMError(); goto error; } @@ -946,7 +946,7 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, configfd, configfd_name); qemuDomainObjExitMonitorWithDriver(driver, vm); } else { - virDomainDevicePCIAddress guestAddr = hostdev->info.addr.pci; + virDomainDevicePCIAddress guestAddr = hostdev->info->addr.pci; qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorAddPCIHostDevice(priv->mon, @@ -954,8 +954,8 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, &guestAddr); qemuDomainObjExitMonitorWithDriver(driver, vm); - hostdev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - memcpy(&hostdev->info.addr.pci, &guestAddr, sizeof(guestAddr)); + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + memcpy(&hostdev->info->addr.pci, &guestAddr, sizeof(guestAddr)); } virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); if (ret < 0) @@ -971,10 +971,10 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, error: if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && - (hostdev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && + (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && releaseaddr && qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - hostdev->info.addr.pci.slot) < 0) + hostdev->info->addr.pci.slot) < 0) VIR_WARN("Unable to release PCI address on host device"); qemuDomainReAttachHostdevDevices(driver, vm->def->name, &hostdev, 1); @@ -2016,14 +2016,14 @@ qemuDomainDetachHostPciDevice(struct qemud_driver *driver, return -1; } - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { + if (qemuIsMultiFunctionDevice(vm->def, detach->info)) { qemuReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device: %s"), dev->data.disk->dst); return -1; } - if (!virDomainDeviceAddressIsValid(&detach->info, + if (!virDomainDeviceAddressIsValid(detach->info, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached without a PCI address")); @@ -2032,9 +2032,9 @@ qemuDomainDetachHostPciDevice(struct qemud_driver *driver, qemuDomainObjEnterMonitorWithDriver(driver, vm); if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - ret = qemuMonitorDelDevice(priv->mon, detach->info.alias); + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); } else { - ret = qemuMonitorRemovePCIDevice(priv->mon, &detach->info.addr.pci); + ret = qemuMonitorRemovePCIDevice(priv->mon, &detach->info->addr.pci); } qemuDomainObjExitMonitorWithDriver(driver, vm); virDomainAuditHostdev(vm, detach, "detach", ret == 0); @@ -2060,7 +2060,7 @@ qemuDomainDetachHostPciDevice(struct qemud_driver *driver, if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && qemuDomainPCIAddressReleaseSlot(priv->pciaddrs, - detach->info.addr.pci.slot) < 0) + detach->info->addr.pci.slot) < 0) VIR_WARN("Unable to release PCI address on host device"); if (vm->def->nhostdevs > 1) { @@ -2129,7 +2129,7 @@ qemuDomainDetachHostUsbDevice(struct qemud_driver *driver, return -1; } - if (!detach->info.alias) { + if (!detach->info->alias) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached without a device alias")); return -1; @@ -2142,7 +2142,7 @@ qemuDomainDetachHostUsbDevice(struct qemud_driver *driver, } qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorDelDevice(priv->mon, detach->info.alias); + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); qemuDomainObjExitMonitorWithDriver(driver, vm); virDomainAuditHostdev(vm, detach, "detach", ret == 0); if (ret < 0) diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index f8390ea..81fc0af 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1076,8 +1076,8 @@ xenParseSxprPCI(virDomainDefPtr def, goto error; } - if (VIR_ALLOC(dev) < 0) - goto no_memory; + if (!(dev = virDomainHostdevDefAlloc())) + goto error; dev->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; dev->managed = 0; diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index e580a3e..6e72aea 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -815,8 +815,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (virStrToLong_i(func, NULL, 16, &funcID) < 0) goto skippci; - if (VIR_ALLOC(hostdev) < 0) - goto no_memory; + if (!(hostdev = virDomainHostdevDefAlloc())) + goto cleanup; hostdev->managed = 0; hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; -- 1.7.7.6
On 02/20/2012 10:10 AM, Laine Stump wrote:
In order to allow for a virDomainHostdevDef that uses the virDomainDeviceInfo of a "higher level" device (such as a virDomainNetDef), this patch changes the virDomainDeviceInfo in the HostdevDef into a virDomainDeviceInfoPtr. Rather than adding checks all over the code to check for a null info, we just guarantee that it is always valid. The new function virDomainHostdevDefAlloc() allocates a virDomainDeviceInfo and plugs it in, and virDomainHostdevDefFree() makes sure it is freed.
There were 4 places allocating virDomainHostdevDefs, all of them parsers of one sort or another, and those have all had their VIR_ALLOC(hostdev) changed to virDomainHostdevDefAlloc(). Other than that, and the new functions, all the rest of the changes are just mechanical removals of "&" or changing "." to "->".
+virDomainHostdevDefPtr virDomainHostdevDefAlloc(void) +{ + virDomainHostdevDefPtr def = NULL; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + return NULL; + } + if (VIR_ALLOC(def->info) < 0) { + VIR_FREE(def); + return NULL;
Missing virReportOOMError() on this path.
@@ -6527,15 +6527,17 @@ cleanup: static virDomainHostdevDefPtr qemuParseCommandLinePCI(const char *val) { - virDomainHostdevDefPtr def = NULL; int bus = 0, slot = 0, func = 0; const char *start; char *end; + virDomainHostdevDefPtr def = virDomainHostdevDefAlloc(); + + if (!def) + goto cleanup;
if (!STRPREFIX(val, "host=")) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unknown PCI device syntax '%s'"), val); - VIR_FREE(def); goto cleanup; }
Incomplete conversion - you nuked this VIR_FREE(def), but not the others on the remaining error paths, nor did you fix the cleanup label to call the correct new function to free things now that HostdevDef contains an embedded pointer. [And I have no idea why the VIR_FREE(def) was there in the first place, given that pre-patch, def was initialized as NULL, making the VIR_FREE a no-op]
@@ -6584,11 +6581,14 @@ cleanup: static virDomainHostdevDefPtr qemuParseCommandLineUSB(const char *val) { - virDomainHostdevDefPtr def = NULL; + virDomainHostdevDefPtr def = virDomainHostdevDefAlloc(); int first = 0, second = 0; const char *start; char *end;
+ if (!def) + goto cleanup; + if (!STRPREFIX(val, "host:")) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unknown USB device syntax '%s'"), val);
Same problem here - you've left a (now-unsafe) VIR_FREE(def) in all the error paths, instead of making cleanup: call a common function that properly frees both def and its contents. Overall, the idea looks reasonable, but you'll need a v2 to fix the memory issues in qemu_command.c. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/20/2012 01:48 PM, Eric Blake wrote:
Overall, the idea looks reasonable, but you'll need a v2 to fix the memory issues in qemu_command.c.
Actually, there were problems with freeing the hostdevdef on error paths in all 4 places virDomainHostdevDefAlloc() was used :-( I don't know what I was thinking... I've updated the patch with the following diff and will repost the full patch.
On 02/20/2012 12:46 PM, Laine Stump wrote:
On 02/20/2012 01:48 PM, Eric Blake wrote:
Overall, the idea looks reasonable, but you'll need a v2 to fix the memory issues in qemu_command.c. Actually, there were problems with freeing the hostdevdef on error paths in all 4 places virDomainHostdevDefAlloc() was used :-( I don't know what I was thinking...
I've updated the patch with the following diff and will repost the full patch.
0001-fix-memory-leaks-in-make-hostdev-info-a-separate-obj.patch
ACK to squashing this in. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
I'm also squashing in this (addition of virDomainHostdevDefClear()) into PATCH 1/6, per the discussion in: https://www.redhat.com/archives/libvir-list/2012-February/msg00895.html --- src/conf/domain_conf.c | 17 ++++++++++++++++- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 18 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c950391..3719097 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1313,12 +1313,27 @@ virDomainHostdevDefPtr virDomainHostdevDefAlloc(void) return def; } -void virDomainHostdevDefFree(virDomainHostdevDefPtr def) +void virDomainHostdevDefClear(virDomainHostdevDefPtr def) { if (!def) return; + /* At this point, any resources in the hostdevdef *except + * def->info* should be freed. Currently there are no such + * resources. + */ + virDomainDeviceInfoFree(def->info); +} + +void virDomainHostdevDefFree(virDomainHostdevDefPtr def) +{ + if (!def) + return; + + /* free all subordinate objects */ + virDomainHostdevDefClear(def); + VIR_FREE(def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1f7f25a..43cc08f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1741,6 +1741,7 @@ void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def); void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); void virDomainVideoDefFree(virDomainVideoDefPtr def); virDomainHostdevDefPtr virDomainHostdevDefAlloc(void); +void virDomainHostdevDefClear(virDomainHostdevDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainHubDefFree(virDomainHubDefPtr def); void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dc55f7b..0869ee6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -351,6 +351,7 @@ virDomainGraphicsSpiceZlibCompressionTypeToString; virDomainGraphicsTypeFromString; virDomainGraphicsTypeToString; virDomainHostdevDefAlloc; +virDomainHostdevDefClear; virDomainHostdevDefFree; virDomainHostdevModeTypeToString; virDomainHostdevSubsysTypeToString; -- 1.7.7.6
This patch is only code movement + adding some forward definitions of typedefs. virDomainHostdevDef (not just a pointer to it, but an actual object) will be needed in virDomainNetDef and virDomainActualNetDef, so it must be relocated earlier in the file. Likewise, virDomainDeviceDef will be needed in virDomainHostdevDef, so it must be moved up even earlier. This, in turn, creates a forward reference problem, but fortunately only with pointers to other device types, so their typedefs can be moved up in the file, eliminating the problem. Also, a DEVICE_TYPE_NONE is added, to indicate that a virDomainDeviceDef doesn't point to a valid device. --- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 212 +++++++++++++++++++++++++++++------------------- 2 files changed, 129 insertions(+), 84 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 137fb73..7f53082 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -123,6 +123,7 @@ VIR_ENUM_IMPL(virDomainLifecycleCrash, VIR_DOMAIN_LIFECYCLE_CRASH_LAST, "coredump-restart") VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, + "none", "disk", "lease", "filesystem", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1f7f25a..25019e7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -44,6 +44,89 @@ # include "virnetdevopenvswitch.h" # include "virnetdevbandwidth.h" +/* forward declarations of all device types, required by + * virDomainDeviceDef + */ +typedef struct _virDomainDiskDef virDomainDiskDef; +typedef virDomainDiskDef *virDomainDiskDefPtr; + +typedef struct _virDomainControllerDef virDomainControllerDef; +typedef virDomainControllerDef *virDomainControllerDefPtr; + +typedef struct _virDomainLeaseDef virDomainLeaseDef; +typedef virDomainLeaseDef *virDomainLeaseDefPtr; + +typedef struct _virDomainFSDef virDomainFSDef; +typedef virDomainFSDef *virDomainFSDefPtr; + +typedef struct _virDomainNetDef virDomainNetDef; +typedef virDomainNetDef *virDomainNetDefPtr; + +typedef struct _virDomainInputDef virDomainInputDef; +typedef virDomainInputDef *virDomainInputDefPtr; + +typedef struct _virDomainSoundDef virDomainSoundDef; +typedef virDomainSoundDef *virDomainSoundDefPtr; + +typedef struct _virDomainVideoDef virDomainVideoDef; +typedef virDomainVideoDef *virDomainVideoDefPtr; + +typedef struct _virDomainHostdevDef virDomainHostdevDef; +typedef virDomainHostdevDef *virDomainHostdevDefPtr; + +typedef struct _virDomainWatchdogDef virDomainWatchdogDef; +typedef virDomainWatchdogDef *virDomainWatchdogDefPtr; + +typedef struct _virDomainGraphicsDef virDomainGraphicsDef; +typedef virDomainGraphicsDef *virDomainGraphicsDefPtr; + +typedef struct _virDomainHubDef virDomainHubDef; +typedef virDomainHubDef *virDomainHubDefPtr; + +typedef struct _virDomainRedirdevDef virDomainRedirdevDef; +typedef virDomainRedirdevDef *virDomainRedirdevDefPtr; + +/* Flags for the 'type' field in next struct */ +enum virDomainDeviceType { + VIR_DOMAIN_DEVICE_NONE = 0, + VIR_DOMAIN_DEVICE_DISK, + VIR_DOMAIN_DEVICE_LEASE, + VIR_DOMAIN_DEVICE_FS, + VIR_DOMAIN_DEVICE_NET, + VIR_DOMAIN_DEVICE_INPUT, + VIR_DOMAIN_DEVICE_SOUND, + VIR_DOMAIN_DEVICE_VIDEO, + VIR_DOMAIN_DEVICE_HOSTDEV, + VIR_DOMAIN_DEVICE_WATCHDOG, + VIR_DOMAIN_DEVICE_CONTROLLER, + VIR_DOMAIN_DEVICE_GRAPHICS, + VIR_DOMAIN_DEVICE_HUB, + VIR_DOMAIN_DEVICE_REDIRDEV, + + VIR_DOMAIN_DEVICE_LAST, +}; + +typedef struct _virDomainDeviceDef virDomainDeviceDef; +typedef virDomainDeviceDef *virDomainDeviceDefPtr; +struct _virDomainDeviceDef { + int type; /* enum virDomainDeviceType */ + union { + virDomainDiskDefPtr disk; + virDomainControllerDefPtr controller; + virDomainLeaseDefPtr lease; + virDomainFSDefPtr fs; + virDomainNetDefPtr net; + virDomainInputDefPtr input; + virDomainSoundDefPtr sound; + virDomainVideoDefPtr video; + virDomainHostdevDefPtr hostdev; + virDomainWatchdogDefPtr watchdog; + virDomainGraphicsDefPtr graphics; + virDomainHubDefPtr hub; + virDomainRedirdevDefPtr redirdev; + } data; +}; + /* Different types of hypervisor */ /* NB: Keep in sync with virDomainVirtTypeToString impl */ enum virDomainVirtType { @@ -243,6 +326,51 @@ struct _virDomainLeaseDef { }; +enum virDomainHostdevMode { + VIR_DOMAIN_HOSTDEV_MODE_SUBSYS, + VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES, + + VIR_DOMAIN_HOSTDEV_MODE_LAST, +}; + +enum virDomainHostdevSubsysType { + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI, + + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST +}; + +/* basic device for direct passthrough */ +typedef struct _virDomainHostdevDef virDomainHostdevDef; +typedef virDomainHostdevDef *virDomainHostdevDefPtr; +struct _virDomainHostdevDef { + int mode; /* enum virDomainHostdevMode */ + unsigned int managed : 1; + union { + struct { + int type; /* enum virDomainHostdevBusType */ + union { + struct { + unsigned bus; + unsigned device; + + unsigned vendor; + unsigned product; + } usb; + virDomainDevicePCIAddress pci; /* host address */ + } u; + } subsys; + struct { + /* TBD: struct capabilities see: + * https://www.redhat.com/archives/libvir-list/2008-July/msg00429.html + */ + int dummy; + } caps; + } source; + virDomainHostdevOrigStates origstates; + virDomainDeviceInfoPtr info; /* Guest address */ +}; + /* Two types of disk backends */ enum virDomainDiskType { VIR_DOMAIN_DISK_TYPE_BLOCK, @@ -1088,50 +1216,6 @@ struct _virDomainGraphicsDef { virDomainGraphicsListenDefPtr listens; }; -enum virDomainHostdevMode { - VIR_DOMAIN_HOSTDEV_MODE_SUBSYS, - VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES, - - VIR_DOMAIN_HOSTDEV_MODE_LAST, -}; - -enum virDomainHostdevSubsysType { - VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB, - VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI, - - VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST -}; - -typedef struct _virDomainHostdevDef virDomainHostdevDef; -typedef virDomainHostdevDef *virDomainHostdevDefPtr; -struct _virDomainHostdevDef { - int mode; /* enum virDomainHostdevMode */ - unsigned int managed : 1; - union { - struct { - int type; /* enum virDomainHostdevBusType */ - union { - struct { - unsigned bus; - unsigned device; - - unsigned vendor; - unsigned product; - } usb; - virDomainDevicePCIAddress pci; /* host address */ - } u; - } subsys; - struct { - /* TBD: struct capabilities see: - * https://www.redhat.com/archives/libvir-list/2008-July/msg00429.html - */ - int dummy; - } caps; - } source; - virDomainHostdevOrigStates origstates; - virDomainDeviceInfoPtr info; /* Guest address */ -}; - enum virDomainRedirdevBus { VIR_DOMAIN_REDIRDEV_BUS_USB, @@ -1175,46 +1259,6 @@ enum virDomainSmbiosMode { VIR_DOMAIN_SMBIOS_LAST }; -/* Flags for the 'type' field in next struct */ -enum virDomainDeviceType { - VIR_DOMAIN_DEVICE_DISK, - VIR_DOMAIN_DEVICE_LEASE, - VIR_DOMAIN_DEVICE_FS, - VIR_DOMAIN_DEVICE_NET, - VIR_DOMAIN_DEVICE_INPUT, - VIR_DOMAIN_DEVICE_SOUND, - VIR_DOMAIN_DEVICE_VIDEO, - VIR_DOMAIN_DEVICE_HOSTDEV, - VIR_DOMAIN_DEVICE_WATCHDOG, - VIR_DOMAIN_DEVICE_CONTROLLER, - VIR_DOMAIN_DEVICE_GRAPHICS, - VIR_DOMAIN_DEVICE_HUB, - VIR_DOMAIN_DEVICE_REDIRDEV, - - VIR_DOMAIN_DEVICE_LAST, -}; - -typedef struct _virDomainDeviceDef virDomainDeviceDef; -typedef virDomainDeviceDef *virDomainDeviceDefPtr; -struct _virDomainDeviceDef { - int type; - union { - virDomainDiskDefPtr disk; - virDomainControllerDefPtr controller; - virDomainLeaseDefPtr lease; - virDomainFSDefPtr fs; - virDomainNetDefPtr net; - virDomainInputDefPtr input; - virDomainSoundDefPtr sound; - virDomainVideoDefPtr video; - virDomainHostdevDefPtr hostdev; - virDomainWatchdogDefPtr watchdog; - virDomainGraphicsDefPtr graphics; - virDomainHubDefPtr hub; - virDomainRedirdevDefPtr redirdev; - } data; -}; - # define VIR_DOMAIN_MAX_BOOT_DEVS 4 -- 1.7.7.6
On 02/20/2012 10:10 AM, Laine Stump wrote:
This patch is only code movement + adding some forward definitions of typedefs.
virDomainHostdevDef (not just a pointer to it, but an actual object) will be needed in virDomainNetDef and virDomainActualNetDef, so it must be relocated earlier in the file.
Likewise, virDomainDeviceDef will be needed in virDomainHostdevDef, so it must be moved up even earlier. This, in turn, creates a forward reference problem, but fortunately only with pointers to other device types, so their typedefs can be moved up in the file, eliminating the problem.
Also, a DEVICE_TYPE_NONE is added, to indicate that a virDomainDeviceDef doesn't point to a valid device.
On the qemu-devel list, I keep hearing the refrain: 'Any time you write a sentence beginning with "Also" in your commit message, that's a sign that you probably should have split it into two patches.' In this particular case, I'm not going to demand a change, but it's food for thought.
+ +/* Flags for the 'type' field in next struct */
Should we specifically call out virDomainDeviceDef, rather than the vague "next struct"? But this is faithful code motion, for now.
+enum virDomainDeviceType {
Back to the question of whether to use enum typedefs. No need to change now, though, as this was just code motion and matches the style in use throughout this particular file. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/20/2012 01:55 PM, Eric Blake wrote:
On 02/20/2012 10:10 AM, Laine Stump wrote:
+ +/* Flags for the 'type' field in next struct */ Should we specifically call out virDomainDeviceDef, rather than the vague "next struct"? But this is faithful code motion, for now.
Even though this patch was (mostly - see my addition of DEVICE_TYPE_NONE) code change, and I hadn't noticed that comment's exact text before, I agree with you, so I'm changing it to specifically say virDomainDeviceDef instead of "next struct".
No code change, movement only. This is necessary to eliminate forward references. --- src/conf/domain_conf.c | 417 ++++++++++++++++++++++++------------------------ 1 files changed, 208 insertions(+), 209 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7f53082..0a4fdee 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2525,6 +2525,214 @@ virDomainParseLegacyDeviceAddress(char *devaddr, return 0; } +static int +virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, + virDomainHostdevDefPtr def) +{ + + int ret = -1; + int got_product, got_vendor; + xmlNodePtr cur; + + /* Product can validly be 0, so we need some extra help to determine + * if it is uninitialized*/ + got_product = 0; + got_vendor = 0; + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "vendor")) { + char *vendor = virXMLPropString(cur, "id"); + + if (vendor) { + got_vendor = 1; + if (virStrToLong_ui(vendor, NULL, 0, + &def->source.subsys.u.usb.vendor) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse vendor id %s"), vendor); + VIR_FREE(vendor); + goto out; + } + VIR_FREE(vendor); + } else { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("usb vendor needs id")); + goto out; + } + } else if (xmlStrEqual(cur->name, BAD_CAST "product")) { + char* product = virXMLPropString(cur, "id"); + + if (product) { + got_product = 1; + if (virStrToLong_ui(product, NULL, 0, + &def->source.subsys.u.usb.product) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse product %s"), + product); + VIR_FREE(product); + goto out; + } + VIR_FREE(product); + } else { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("usb product needs id")); + goto out; + } + } else if (xmlStrEqual(cur->name, BAD_CAST "address")) { + char *bus, *device; + + bus = virXMLPropString(cur, "bus"); + if (bus) { + if (virStrToLong_ui(bus, NULL, 0, + &def->source.subsys.u.usb.bus) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse bus %s"), bus); + VIR_FREE(bus); + goto out; + } + VIR_FREE(bus); + } else { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("usb address needs bus id")); + goto out; + } + + device = virXMLPropString(cur, "device"); + if (device) { + if (virStrToLong_ui(device, NULL, 0, + &def->source.subsys.u.usb.device) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse device %s"), + device); + VIR_FREE(device); + goto out; + } + VIR_FREE(device); + } else { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("usb address needs device id")); + goto out; + } + } else { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown usb source type '%s'"), + cur->name); + goto out; + } + } + cur = cur->next; + } + + if (got_vendor && def->source.subsys.u.usb.vendor == 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("vendor cannot be 0.")); + goto out; + } + + if (!got_vendor && got_product) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing vendor")); + goto out; + } + if (got_vendor && !got_product) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing product")); + goto out; + } + + ret = 0; +out: + return ret; +} + +/* The internal XML for host PCI device's original states: + * + * <origstates> + * <unbind/> + * <removeslot/> + * <reprobe/> + * </origstates> + */ +static int +virDomainHostdevSubsysPciOrigStatesDefParseXML(const xmlNodePtr node, + virDomainHostdevOrigStatesPtr def) +{ + xmlNodePtr cur; + cur = node->children; + + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "unbind")) { + def->states.pci.unbind_from_stub = 1; + } else if (xmlStrEqual(cur->name, BAD_CAST "removeslot")) { + def->states.pci.remove_slot = 1; + } else if (xmlStrEqual(cur->name, BAD_CAST "reprobe")) { + def->states.pci.reprobe = 1; + } else { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported element '%s' of 'origstates'"), + cur->name); + return -1; + } + } + cur = cur->next; + } + + return 0; +} + +static int +virDomainHostdevSubsysPciDefParseXML(const xmlNodePtr node, + virDomainHostdevDefPtr def, + unsigned int flags) +{ + int ret = -1; + xmlNodePtr cur; + + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "address")) { + virDomainDevicePCIAddressPtr addr = + &def->source.subsys.u.pci; + + if (virDomainDevicePCIAddressParseXML(cur, addr) < 0) + goto out; + } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && + xmlStrEqual(cur->name, BAD_CAST "state")) { + /* Legacy back-compat. Don't add any more attributes here */ + char *devaddr = virXMLPropString(cur, "devaddr"); + if (devaddr && + virDomainParseLegacyDeviceAddress(devaddr, + &def->info->addr.pci) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse devaddr parameter '%s'"), + devaddr); + VIR_FREE(devaddr); + goto out; + } + def->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + } else if ((flags & VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES) && + xmlStrEqual(cur->name, BAD_CAST "origstates")) { + virDomainHostdevOrigStatesPtr states = &def->origstates; + if (virDomainHostdevSubsysPciOrigStatesDefParseXML(cur, states) < 0) + goto out; + } else { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown pci source type '%s'"), + cur->name); + goto out; + } + } + cur = cur->next; + } + + ret = 0; +out: + return ret; +} + int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def) { @@ -6107,215 +6315,6 @@ error: return NULL; } -static int -virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node, - virDomainHostdevDefPtr def) -{ - - int ret = -1; - int got_product, got_vendor; - xmlNodePtr cur; - - /* Product can validly be 0, so we need some extra help to determine - * if it is uninitialized*/ - got_product = 0; - got_vendor = 0; - - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (xmlStrEqual(cur->name, BAD_CAST "vendor")) { - char *vendor = virXMLPropString(cur, "id"); - - if (vendor) { - got_vendor = 1; - if (virStrToLong_ui(vendor, NULL, 0, - &def->source.subsys.u.usb.vendor) < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse vendor id %s"), vendor); - VIR_FREE(vendor); - goto out; - } - VIR_FREE(vendor); - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("usb vendor needs id")); - goto out; - } - } else if (xmlStrEqual(cur->name, BAD_CAST "product")) { - char* product = virXMLPropString(cur, "id"); - - if (product) { - got_product = 1; - if (virStrToLong_ui(product, NULL, 0, - &def->source.subsys.u.usb.product) < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse product %s"), - product); - VIR_FREE(product); - goto out; - } - VIR_FREE(product); - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("usb product needs id")); - goto out; - } - } else if (xmlStrEqual(cur->name, BAD_CAST "address")) { - char *bus, *device; - - bus = virXMLPropString(cur, "bus"); - if (bus) { - if (virStrToLong_ui(bus, NULL, 0, - &def->source.subsys.u.usb.bus) < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse bus %s"), bus); - VIR_FREE(bus); - goto out; - } - VIR_FREE(bus); - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("usb address needs bus id")); - goto out; - } - - device = virXMLPropString(cur, "device"); - if (device) { - if (virStrToLong_ui(device, NULL, 0, - &def->source.subsys.u.usb.device) < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse device %s"), - device); - VIR_FREE(device); - goto out; - } - VIR_FREE(device); - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("usb address needs device id")); - goto out; - } - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown usb source type '%s'"), - cur->name); - goto out; - } - } - cur = cur->next; - } - - if (got_vendor && def->source.subsys.u.usb.vendor == 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("vendor cannot be 0.")); - goto out; - } - - if (!got_vendor && got_product) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing vendor")); - goto out; - } - if (got_vendor && !got_product) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing product")); - goto out; - } - - ret = 0; -out: - return ret; -} - -/* The internal XML for host PCI device's original states: - * - * <origstates> - * <unbind/> - * <removeslot/> - * <reprobe/> - * </origstates> - */ -static int -virDomainHostdevSubsysPciOrigStatesDefParseXML(const xmlNodePtr node, - virDomainHostdevOrigStatesPtr def) -{ - xmlNodePtr cur; - cur = node->children; - - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (xmlStrEqual(cur->name, BAD_CAST "unbind")) { - def->states.pci.unbind_from_stub = 1; - } else if (xmlStrEqual(cur->name, BAD_CAST "removeslot")) { - def->states.pci.remove_slot = 1; - } else if (xmlStrEqual(cur->name, BAD_CAST "reprobe")) { - def->states.pci.reprobe = 1; - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported element '%s' of 'origstates'"), - cur->name); - return -1; - } - } - cur = cur->next; - } - - return 0; -} - -static int -virDomainHostdevSubsysPciDefParseXML(const xmlNodePtr node, - virDomainHostdevDefPtr def, - unsigned int flags) -{ - int ret = -1; - xmlNodePtr cur; - - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (xmlStrEqual(cur->name, BAD_CAST "address")) { - virDomainDevicePCIAddressPtr addr = - &def->source.subsys.u.pci; - - if (virDomainDevicePCIAddressParseXML(cur, addr) < 0) - goto out; - } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && - xmlStrEqual(cur->name, BAD_CAST "state")) { - /* Legacy back-compat. Don't add any more attributes here */ - char *devaddr = virXMLPropString(cur, "devaddr"); - if (devaddr && - virDomainParseLegacyDeviceAddress(devaddr, - &def->info->addr.pci) < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to parse devaddr parameter '%s'"), - devaddr); - VIR_FREE(devaddr); - goto out; - } - def->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - } else if ((flags & VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES) && - xmlStrEqual(cur->name, BAD_CAST "origstates")) { - virDomainHostdevOrigStatesPtr states = &def->origstates; - if (virDomainHostdevSubsysPciOrigStatesDefParseXML(cur, states) < 0) - goto out; - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown pci source type '%s'"), - cur->name); - goto out; - } - } - cur = cur->next; - } - - ret = 0; -out: - return ret; -} - - static virDomainHostdevDefPtr virDomainHostdevDefParseXML(const xmlNodePtr node, virBitmapPtr bootMap, -- 1.7.7.6
On 02/20/2012 10:10 AM, Laine Stump wrote:
No code change, movement only. This is necessary to eliminate forward references. --- src/conf/domain_conf.c | 417 ++++++++++++++++++++++++------------------------ 1 files changed, 208 insertions(+), 209 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
In an upcoming patch, virDomainNetDef will acquire a virDomainHostdevDef, and the <interface> XML will take on some of the elements of a <hostdev>. To avoid duplicating the code for parsing and formatting the <source> element (which will be nearly identical in these two cases), this patch factors those parts out of the HostdevDef's parse and format functions, and puts them into separate helper functions that are now called by the HostdevDef parser/formatter, and will soon be called by the NetDef parser/formatter. One change in behavior - previously virDomainHostdevDefParseXML() had diverged from current common coding practice by logging an error and failing if it found any subelements of <hostdev> other than those it understood (standard libvirt practice is to ignore/discard unknown elements and attributes during parse). The new helper function ignores unknown elements, and thus so does the new virDomainHostdevDefParseXML. --- src/conf/domain_conf.c | 262 +++++++++++++++++++++++++++++------------------- 1 files changed, 159 insertions(+), 103 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0a4fdee..004cba7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2733,6 +2733,89 @@ out: return ret; } +static int +virDomainHostdevPartsParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + char *mode, char *type, + virDomainHostdevDefPtr def, + unsigned int flags) +{ + xmlNodePtr sourcenode; + char *managed = NULL; + int ret = -1; + + /* @mode is passed in separately from the caller, since an + * 'intelligent hostdev' has no place for 'mode' in the XML (it is + * always 'subsys'). + */ + if (mode) { + if ((def->mode=virDomainHostdevModeTypeFromString(mode)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown hostdev mode '%s'"), mode); + goto error; + } + } else { + def->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; + } + + /* @managed can be read from the xml document - it is always an + * attribute of the toplevel element, no matter what type of + * element that might be (pure hostdev, or higher level device + * (e.g. <interface>) with type='hostdev') + */ + if ((managed = virXMLPropString(node, "managed"))!= NULL) { + if (STREQ(managed, "yes")) + def->managed = 1; + } + + /* @type is passed in from the caller rather than read from the + * xml document, because it is specificed in different places for + * different kinds of defs - it is an attribute of + * <source>/<address> for an intelligent hostdev (<interface>), + * but an attribute of the toplevel element for a standard + * <hostdev>. (the functions we're going to call expect address + * type to already be known). + */ + if (type) { + if ((def->source.subsys.type + = virDomainHostdevSubsysTypeFromString(type)) < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown host device source address type '%s'"), + type); + goto error; + } + } else { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("missing source address type")); + goto error; + } + + if (!(sourcenode = virXPathNode("./source", ctxt))) { + virDomainReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing <source> element in hostdev device")); + goto error; + } + switch (def->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + if (virDomainHostdevSubsysPciDefParseXML(sourcenode, def, flags) < 0) + goto error; + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + if (virDomainHostdevSubsysUsbDefParseXML(sourcenode, def) < 0) + goto error; + break; + default: + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("address type='%s' not supported in hostdev interfaces"), + virDomainHostdevSubsysTypeToString(def->source.subsys.type)); + goto error; + } + ret = 0; +error: + VIR_FREE(managed); + return ret; +} + int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def) { @@ -6317,76 +6400,23 @@ error: static virDomainHostdevDefPtr virDomainHostdevDefParseXML(const xmlNodePtr node, + xmlXPathContextPtr ctxt, virBitmapPtr bootMap, unsigned int flags) { - xmlNodePtr cur; virDomainHostdevDefPtr def; - char *mode, *type = NULL, *managed = NULL; - - if (!(def = virDomainHostdevDefAlloc())) - return NULL; + xmlNodePtr save = ctxt->node; + char *mode = virXMLPropString(node, "mode"); + char *type = virXMLPropString(node, "type"); - mode = virXMLPropString(node, "mode"); - if (mode) { - if ((def->mode=virDomainHostdevModeTypeFromString(mode)) < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown hostdev mode '%s'"), mode); - goto error; - } - } else { - def->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS; - } + ctxt->node = node; - type = virXMLPropString(node, "type"); - if (type) { - if ((def->source.subsys.type = virDomainHostdevSubsysTypeFromString(type)) < 0) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown host device type '%s'"), type); - goto error; - } - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing type in hostdev")); + if (!(def = virDomainHostdevDefAlloc())) goto error; - } - managed = virXMLPropString(node, "managed"); - if (managed != NULL) { - if (STREQ(managed, "yes")) - def->managed = 1; - VIR_FREE(managed); - } - - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (xmlStrEqual(cur->name, BAD_CAST "source")) { - if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { - if (virDomainHostdevSubsysUsbDefParseXML(cur, def) < 0) - goto error; - } - if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { - if (virDomainHostdevSubsysPciDefParseXML(cur, def, flags) < 0) - goto error; - } - } else if (xmlStrEqual(cur->name, BAD_CAST "address")) { - /* address is parsed as part of virDomainDeviceInfoParseXML */ - } else if (xmlStrEqual(cur->name, BAD_CAST "alias")) { - /* alias is parsed as part of virDomainDeviceInfoParseXML */ - } else if (xmlStrEqual(cur->name, BAD_CAST "boot")) { - /* boot is parsed as part of virDomainDeviceInfoParseXML */ - } else if (xmlStrEqual(cur->name, BAD_CAST "rom")) { - /* rombar is parsed as part of virDomainDeviceInfoParseXML */ - } else { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown node %s"), cur->name); - } - } - cur = cur->next; - } + /* parse managed/mode/type, and the <source> element */ + if (virDomainHostdevPartsParse(node, ctxt, mode, type, def, flags) < 0) + goto error; if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { if (virDomainDeviceInfoParseXML(node, bootMap, def->info, @@ -6411,6 +6441,7 @@ virDomainHostdevDefParseXML(const xmlNodePtr node, cleanup: VIR_FREE(type); VIR_FREE(mode); + ctxt->node = save; return def; error: @@ -6577,7 +6608,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, goto error; } else if (xmlStrEqual(node->name, BAD_CAST "hostdev")) { dev->type = VIR_DOMAIN_DEVICE_HOSTDEV; - if (!(dev->data.hostdev = virDomainHostdevDefParseXML(node, NULL, + if (!(dev->data.hostdev = virDomainHostdevDefParseXML(node, ctxt, NULL, flags))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "controller")) { @@ -8105,9 +8136,9 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (n && VIR_ALLOC_N(def->hostdevs, n) < 0) goto no_memory; for (i = 0 ; i < n ; i++) { - virDomainHostdevDefPtr hostdev = virDomainHostdevDefParseXML(nodes[i], - bootMap, - flags); + virDomainHostdevDefPtr hostdev; + + hostdev = virDomainHostdevDefParseXML(nodes[i], ctxt, bootMap, flags); if (!hostdev) goto error; @@ -10429,6 +10460,64 @@ virDomainFSDefFormat(virBufferPtr buf, } static int +virDomainHostdevSourceFormat(virBufferPtr buf, + virDomainHostdevDefPtr def, + unsigned int flags, + bool includeTypeInAddr) +{ + virBufferAddLit(buf, "<source>\n"); + switch (def->source.subsys.type) + { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + if (def->source.subsys.u.usb.vendor) { + virBufferAsprintf(buf, " <vendor id='0x%.4x'/>\n", + def->source.subsys.u.usb.vendor); + virBufferAsprintf(buf, " <product id='0x%.4x'/>\n", + def->source.subsys.u.usb.product); + } + if (def->source.subsys.u.usb.bus || + def->source.subsys.u.usb.device) { + virBufferAsprintf(buf, " <address %sbus='%d' device='%d'/>\n", + includeTypeInAddr ? "type='usb' " : "", + def->source.subsys.u.usb.bus, + def->source.subsys.u.usb.device); + } + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + virBufferAsprintf(buf, " <address %sdomain='0x%.4x' bus='0x%.2x' " + "slot='0x%.2x' function='0x%.1x'/>\n", + includeTypeInAddr ? "type='pci' " : "", + def->source.subsys.u.pci.domain, + def->source.subsys.u.pci.bus, + def->source.subsys.u.pci.slot, + def->source.subsys.u.pci.function); + + if ((flags & VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES) && + (def->origstates.states.pci.unbind_from_stub || + def->origstates.states.pci.remove_slot || + def->origstates.states.pci.reprobe)) { + virBufferAddLit(buf, " <origstates>\n"); + if (def->origstates.states.pci.unbind_from_stub) + virBufferAddLit(buf, " <unbind/>\n"); + if (def->origstates.states.pci.remove_slot) + virBufferAddLit(buf, " <removeslot/>\n"); + if (def->origstates.states.pci.reprobe) + virBufferAddLit(buf, " <reprobe/>\n"); + virBufferAddLit(buf, " </origstates>\n"); + } + break; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected hostdev type %d"), + def->source.subsys.type); + return -1; + } + + virBufferAddLit(buf, "</source>\n"); + return 0; +} + +static int virDomainActualNetDefFormat(virBufferPtr buf, virDomainActualNetDefPtr def) { @@ -11446,44 +11535,11 @@ virDomainHostdevDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <hostdev mode='%s' type='%s' managed='%s'>\n", mode, type, def->managed ? "yes" : "no"); - virBufferAddLit(buf, " <source>\n"); - if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { - if (def->source.subsys.u.usb.vendor) { - virBufferAsprintf(buf, " <vendor id='0x%.4x'/>\n", - def->source.subsys.u.usb.vendor); - virBufferAsprintf(buf, " <product id='0x%.4x'/>\n", - def->source.subsys.u.usb.product); - } - if (def->source.subsys.u.usb.bus || - def->source.subsys.u.usb.device) - virBufferAsprintf(buf, " <address bus='%d' device='%d'/>\n", - def->source.subsys.u.usb.bus, - def->source.subsys.u.usb.device); - } else if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) { - virBufferAsprintf(buf, " <address domain='0x%.4x' bus='0x%.2x' " - "slot='0x%.2x' function='0x%.1x'/>\n", - def->source.subsys.u.pci.domain, - def->source.subsys.u.pci.bus, - def->source.subsys.u.pci.slot, - def->source.subsys.u.pci.function); - - if ((flags & VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES) && - (def->origstates.states.pci.unbind_from_stub || - def->origstates.states.pci.remove_slot || - def->origstates.states.pci.reprobe)) { - virBufferAddLit(buf, " <origstates>\n"); - if (def->origstates.states.pci.unbind_from_stub) - virBufferAddLit(buf, " <unbind/>\n"); - if (def->origstates.states.pci.remove_slot) - virBufferAddLit(buf, " <removeslot/>\n"); - if (def->origstates.states.pci.reprobe) - virBufferAddLit(buf, " <reprobe/>\n"); - virBufferAddLit(buf, " </origstates>\n"); - } - } - - virBufferAddLit(buf, " </source>\n"); + virBufferAdjustIndent(buf, 6); + if (virDomainHostdevSourceFormat(buf, def, flags, false) < 0) + return -1; + virBufferAdjustIndent(buf, -6); if (virDomainDeviceInfoFormat(buf, def->info, flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT -- 1.7.7.6
On 02/20/2012 10:10 AM, Laine Stump wrote:
In an upcoming patch, virDomainNetDef will acquire a virDomainHostdevDef, and the <interface> XML will take on some of the elements of a <hostdev>. To avoid duplicating the code for parsing and formatting the <source> element (which will be nearly identical in these two cases), this patch factors those parts out of the HostdevDef's parse and format functions, and puts them into separate helper functions that are now called by the HostdevDef parser/formatter, and will soon be called by the NetDef parser/formatter.
One change in behavior - previously virDomainHostdevDefParseXML() had diverged from current common coding practice by logging an error and failing if it found any subelements of <hostdev> other than those it understood (standard libvirt practice is to ignore/discard unknown elements and attributes during parse). The new helper function ignores unknown elements, and thus so does the new virDomainHostdevDefParseXML.
Seems like a reasonable change in behavior.
+static int +virDomainHostdevPartsParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + char *mode, char *type,
I'd use 'const char *' for both of these parameters.
+ + /* @type is passed in from the caller rather than read from the + * xml document, because it is specificed in different places for
s/specificed/specified/ ACK with those nits fixed. At this point, I'm comfortable if you feel like pushing patches 1-4 (obviously, with the v2 fixes to patch 1) to get them out of the way, as they seem like reasonable cleanups whether or not the rest of your code is complete, and doing it now will reduce the likelihood of merge conflicts if someone else touches the same areas of code. But it's probably better to hold off on 5 and 6 until we have wired up the code to use the refactorization. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The parent can be any type of device. It defaults to type=none, and a NULL pointer. The intent is that if a hostdevdef is contained in the def for a higher level device (e.g. virDomainNetDef), hostdev->parent will point to the higher level device, and type will be set to that type of device. This way, during attach and detach of the device, parent can be checked, and appropriate callouts made to do higher level device initialization (e.g. setting MAC address). Also, although these hostdevs with parents will be added to a domain's hostdevs list, they will be treated slightly differently when traversing the list, e.g. virDomainHostdefDefFree for a hostdev that has a parent doesn't need to be called (and will be a NOP); it will simply be removed from the list (since the parent device object is in its own type-specific list, and will be freed from there). --- src/conf/domain_conf.c | 6 ++++++ src/conf/domain_conf.h | 1 + 2 files changed, 7 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 004cba7..12d48fb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1318,6 +1318,12 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def) if (!def) return; + /* if there is a parent device object, it will handle freeing the + * memory (including the info). + */ + if (def->parent.type != VIR_DOMAIN_DEVICE_NONE) + return; + virDomainDeviceInfoFree(def->info); VIR_FREE(def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 25019e7..924c7ec 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -344,6 +344,7 @@ enum virDomainHostdevSubsysType { typedef struct _virDomainHostdevDef virDomainHostdevDef; typedef virDomainHostdevDef *virDomainHostdevDefPtr; struct _virDomainHostdevDef { + virDomainDeviceDef parent; /* higher level Def containing this */ int mode; /* enum virDomainHostdevMode */ unsigned int managed : 1; union { -- 1.7.7.6
On 02/20/2012 10:10 AM, Laine Stump wrote:
The parent can be any type of device. It defaults to type=none, and a NULL pointer. The intent is that if a hostdevdef is contained in the def for a higher level device (e.g. virDomainNetDef), hostdev->parent will point to the higher level device, and type will be set to that type of device. This way, during attach and detach of the device, parent can be checked, and appropriate callouts made to do higher level device initialization (e.g. setting MAC address).
Also, although these hostdevs with parents will be added to a domain's hostdevs list, they will be treated slightly differently when traversing the list, e.g. virDomainHostdefDefFree for a hostdev that has a parent doesn't need to be called (and will be a NOP); it will simply be removed from the list (since the parent device object is in its own type-specific list, and will be freed from there). --- src/conf/domain_conf.c | 6 ++++++ src/conf/domain_conf.h | 1 + 2 files changed, 7 insertions(+), 0 deletions(-)
Slick. I can see where this is going - we can easily traverse all passthrough devices, and we can also use the backpointer to easily tell which passthrough devices are associated with a higher-level device that libvirt actually knows more about than just an opaque host device.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 004cba7..12d48fb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1318,6 +1318,12 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def) if (!def) return;
+ /* if there is a parent device object, it will handle freeing the + * memory (including the info). + */ + if (def->parent.type != VIR_DOMAIN_DEVICE_NONE) + return; + virDomainDeviceInfoFree(def->info); VIR_FREE(def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 25019e7..924c7ec 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -344,6 +344,7 @@ enum virDomainHostdevSubsysType { typedef struct _virDomainHostdevDef virDomainHostdevDef; typedef virDomainHostdevDef *virDomainHostdevDefPtr; struct _virDomainHostdevDef { + virDomainDeviceDef parent; /* higher level Def containing this */
However, that makes virDomainHostdevDef a rather large struct, since it is including an entire virDomainDeviceDef even if it is not otherwise used. Is it any better to keep the back pointer as a pointer, along the lines of: if (!def->parent) return; struct _virDomainHostdefDef { virDomainDeviceDefPtr parent; I guess I'll see more when I review patch 6, so no decision on ack yet. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/20/2012 05:02 PM, Eric Blake wrote:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 004cba7..12d48fb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1318,6 +1318,12 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def) if (!def) return;
+ /* if there is a parent device object, it will handle freeing the + * memory (including the info). + */ + if (def->parent.type != VIR_DOMAIN_DEVICE_NONE) + return; + virDomainDeviceInfoFree(def->info); VIR_FREE(def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 25019e7..924c7ec 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -344,6 +344,7 @@ enum virDomainHostdevSubsysType { typedef struct _virDomainHostdevDef virDomainHostdevDef; typedef virDomainHostdevDef *virDomainHostdevDefPtr; struct _virDomainHostdevDef { + virDomainDeviceDef parent; /* higher level Def containing this */ However, that makes virDomainHostdevDef a rather large struct, since it is including an entire virDomainDeviceDef even if it is not otherwise used. Is it any better to keep the back pointer as a pointer, along the lines of:
if (!def->parent) return;
struct _virDomainHostdefDef { virDomainDeviceDefPtr parent;
virDomainDeviceDef *looks* big, but it's really just one int and one pointer (everything after "int type" is a union of several different pointer types). Making parent a virDomainDeviceDefPtr only saves sizeof(int), but introduces an extra level of indirection and the possibility of a memory leak, so I think it's best left as it is.
This is the new interface type that sets up a PCI/USB network device to be assigned to the guest with PCI/USB passthrough after initializing some network device-specific things from the config (e.g. MAC address, virtualport profile parameters). Here is an example of the syntax: <interface type='hostdev' managed='yes'> <source> <address type='pci' domain='0' bus='0' slot='4' function='0'/> </source> <mac address='00:11:22:33:44:55'/> <address type='pci' domain='0' bus='0' slot='7' function='0'/> </interface> This would assign the PCI card from bus 0 slot 4 function 0 on the host, to bus 0 slot 7 function 0 on the guest, but would first set the MAC address of the card to 00:11:22:33:44:55. Although it's not expected to be used very much, usb network hostdevs are also supported for completeness. <source> syntax is identical to that for plain <hostdev> devices, except that the <address> element should have "type='usb'" added if it's specified: <interface type='hostdev'> <source> <address type='usb' bus='0' device='4'/> </source> <mac address='00:11:22:33:44:55'/> </interface> If the vendor/product form of usb specification is used, type='usb' is implied: <interface type='hostdev'> <source> <vendor id='0x0012'/> <product id='0x24dd'/> </source> <mac address='00:11:22:33:44:55'/> </interface> --- docs/formatdomain.html.in | 41 +++++ docs/schemas/domaincommon.rng | 50 ++++++ src/conf/domain_conf.c | 158 ++++++++++++++++++-- src/conf/domain_conf.h | 10 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 1 + src/uml/uml_conf.c | 5 + src/xenxs/xen_sxpr.c | 1 + .../qemuxml2argvdata/qemuxml2argv-net-hostdev.xml | 48 ++++++ tests/qemuxml2xmltest.c | 1 + 10 files changed, 301 insertions(+), 15 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 5305f82..535b56d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2295,6 +2295,47 @@ ... </pre> + + <h5><a name="elementsNICSHostdev">PCI/USB Passthrough</a></h5> + + <p> + A PCI or USB network device (specified by the <source> + element) is directly assigned to the guest using generic device + passthrough, after first optionally setting the device's MAC + address to the configured value, and associating the device with + a VEPA or 802.1Qgh capable switch using an optionally specified + %lt;virtualport%gt; element (see the examples of virtualport + given above for type='direct' network devices). + <span class="since">Since 0.9.11</span> + </p> + + <p> + Note that this "intelligent passthrough" of network devices is + very similar to the functionality of a standard <hostdev> + device, the difference being that this method allows specifying + a MAC address and <virtualport> for the passed-through + device. If these capabilities are not required, of if you are + using a version of libvirt older than 0.9.11, you should use + standard <hostdev> to assign the device to the guest + instead of <interface type='hostdev'/>. + </p> + +<pre> + ... + <devices> + <interface type='hostdev'> + <source> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </source> + <mac address='52:54:00:6d:90:02'> + <virtualport type='802.1Qbh'> + <parameters profileid='finance'/> + </virtualport> + </interface> + </devices> + ...</pre> + + <h5><a name="elementsNICSMulticast">Multicast tunnel</a></h5> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e276a92..f52f8e3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1414,6 +1414,56 @@ </optional> </interleave> </group> + <group> + <attribute name="type"> + <value>hostdev</value> + </attribute> + <optional> + <attribute name="managed"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> + <interleave> + <element name="source"> + <choice> + <group> + <ref name="usbproduct"/> + <optional> + <ref name="usbaddress"/> + </optional> + </group> + <element name="address"> + <choice> + <group> + <attribute name="type"> + <value>pci</value> + </attribute> + <ref name="pciaddress"/> + </group> + <group> + <attribute name="type"> + <value>usb</value> + </attribute> + <attribute name="bus"> + <ref name="usbAddr"/> + </attribute> + <attribute name="device"> + <ref name="usbPort"/> + </attribute> + </group> + </choice> + </element> + </choice> + </element> + <optional> + <ref name="virtualPortProfile"/> + </optional> + <ref name="interface-options"/> + </interleave> + </group> </choice> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 12d48fb..c0503f4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -281,7 +281,8 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "network", "bridge", "internal", - "direct") + "direct", + "hostdev") VIR_ENUM_IMPL(virDomainNetBackend, VIR_DOMAIN_NET_BACKEND_TYPE_LAST, "default", @@ -966,6 +967,12 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def) VIR_FREE(def->data.direct.linkdev); VIR_FREE(def->data.direct.virtPortProfile); break; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + /* currently there is nothing in a virDomainHostdevDef + * that requires freeing. + */ + VIR_FREE(def->data.hostdev.virtPortProfile); + break; default: break; } @@ -1016,6 +1023,13 @@ void virDomainNetDefFree(virDomainNetDefPtr def) VIR_FREE(def->data.direct.virtPortProfile); break; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + /* currently there is nothing in a virDomainHostdevDef + * that requires freeing. + */ + VIR_FREE(def->data.hostdev.virtPortProfile); + break; + case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_LAST: break; @@ -4018,7 +4032,9 @@ cleanup: static int virDomainActualNetDefParseXML(xmlNodePtr node, xmlXPathContextPtr ctxt, - virDomainActualNetDefPtr *def) + virDomainNetDefPtr parent, + virDomainActualNetDefPtr *def, + unsigned int flags) { virDomainActualNetDefPtr actual = NULL; int ret = -1; @@ -4026,6 +4042,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, xmlNodePtr bandwidth_node = NULL; char *type = NULL; char *mode = NULL; + char *addrtype = NULL; if (VIR_ALLOC(actual) < 0) { virReportOOMError(); @@ -4047,6 +4064,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, } if (actual->type != VIR_DOMAIN_NET_TYPE_BRIDGE && actual->type != VIR_DOMAIN_NET_TYPE_DIRECT && + actual->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && actual->type != VIR_DOMAIN_NET_TYPE_NETWORK) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported type '%s' in interface's <actual> element"), @@ -4082,6 +4100,31 @@ virDomainActualNetDefParseXML(xmlNodePtr node, (!(actual->data.direct.virtPortProfile = virNetDevVPortProfileParse(virtPortNode)))) goto error; + } else if (actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt); + virDomainHostdevDefPtr hostdev = &actual->data.hostdev.def; + + hostdev->parent.type = VIR_DOMAIN_DEVICE_NET; + hostdev->parent.data.net = parent; + hostdev->info = &parent->info; + /* The helper function expects type to already be found and + * passed in as a string, since it is in a different place in + * NetDef vs HostdevDef. + */ + addrtype = virXPathString("string(./source/address/@type)", ctxt); + if ((!addrtype) && virXPathNode("./source/vendor", ctxt)) + addrtype = strdup("usb"); /* source/vendor implies usb device */ + + if (virDomainHostdevPartsParse(node, ctxt, NULL, addrtype, + hostdev, flags) < 0) { + goto error; + } + + if (virtPortNode && + (!(actual->data.hostdev.virtPortProfile = + virNetDevVPortProfileParse(virtPortNode)))) { + goto error; + } } bandwidth_node = virXPathNode("./bandwidth", ctxt); @@ -4095,6 +4138,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, error: VIR_FREE(type); VIR_FREE(mode); + VIR_FREE(addrtype); virDomainActualNetDefFree(actual); ctxt->node = save_ctxt; @@ -4116,6 +4160,7 @@ virDomainNetDefParseXML(virCapsPtr caps, unsigned int flags) { virDomainNetDefPtr def; + virDomainHostdevDefPtr hostdev; xmlNodePtr cur; char *macaddr = NULL; char *type = NULL; @@ -4137,6 +4182,7 @@ virDomainNetDefParseXML(virCapsPtr caps, char *devaddr = NULL; char *mode = NULL; char *linkstate = NULL; + char *addrtype = NULL; virNWFilterHashTablePtr filterparams = NULL; virNetDevVPortProfilePtr virtPort = NULL; virDomainActualNetDefPtr actual = NULL; @@ -4189,7 +4235,8 @@ virDomainNetDefParseXML(virCapsPtr caps, } else if ((virtPort == NULL) && ((def->type == VIR_DOMAIN_NET_TYPE_DIRECT) || (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) || - (def->type == VIR_DOMAIN_NET_TYPE_BRIDGE)) && + (def->type == VIR_DOMAIN_NET_TYPE_BRIDGE) || + (def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)) && xmlStrEqual(cur->name, BAD_CAST "virtualport")) { if (!(virtPort = virNetDevVPortProfileParse(cur))) goto error; @@ -4241,8 +4288,10 @@ virDomainNetDefParseXML(virCapsPtr caps, (flags & VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET) && (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) && xmlStrEqual(cur->name, BAD_CAST "actual")) { - if (virDomainActualNetDefParseXML(cur, ctxt, &actual) < 0) + if (virDomainActualNetDefParseXML(cur, ctxt, def, + &actual, flags) < 0) { goto error; + } } else if (xmlStrEqual(cur->name, BAD_CAST "bandwidth")) { if (!(def->bandwidth = virNetDevBandwidthParse(cur))) goto error; @@ -4397,6 +4446,27 @@ virDomainNetDefParseXML(virCapsPtr caps, break; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + hostdev = &def->data.hostdev.def; + hostdev->parent.type = VIR_DOMAIN_DEVICE_NET; + hostdev->parent.data.net = def; + hostdev->info = &def->info; + /* The helper function expects type to already be found and + * passed in as a string, since it is in a different place in + * NetDef vs HostdevDef. + */ + addrtype = virXPathString("string(./source/address/@type)", ctxt); + if ((!addrtype) && virXPathNode("./source/vendor", ctxt)) + addrtype = strdup("usb"); /* source/vendor implies usb device */ + + if (virDomainHostdevPartsParse(node, ctxt, NULL, addrtype, + hostdev, flags) < 0) { + goto error; + } + def->data.hostdev.virtPortProfile = virtPort; + virtPort = NULL; + break; + case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_LAST: break; @@ -4532,6 +4602,7 @@ cleanup: VIR_FREE(devaddr); VIR_FREE(mode); VIR_FREE(linkstate); + VIR_FREE(addrtype); virNWFilterHashTableFree(filterparams); return def; @@ -10525,7 +10596,8 @@ virDomainHostdevSourceFormat(virBufferPtr buf, static int virDomainActualNetDefFormat(virBufferPtr buf, - virDomainActualNetDefPtr def) + virDomainActualNetDefPtr def, + unsigned int flags) { int ret = -1; const char *type; @@ -10541,14 +10613,12 @@ virDomainActualNetDefFormat(virBufferPtr buf, return ret; } - if (def->type != VIR_DOMAIN_NET_TYPE_BRIDGE && - def->type != VIR_DOMAIN_NET_TYPE_DIRECT && - def->type != VIR_DOMAIN_NET_TYPE_NETWORK) { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected net type %s"), type); - goto error; + virBufferAsprintf(buf, " <actual type='%s'", type); + if ((def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) && + def->data.hostdev.def.managed) { + virBufferAddLit(buf, " managed='yes'"); } - virBufferAsprintf(buf, " <actual type='%s'>\n", type); + virBufferAddLit(buf, ">\n"); switch (def->type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -10581,8 +10651,31 @@ virDomainActualNetDefFormat(virBufferPtr buf, goto error; virBufferAdjustIndent(buf, -8); break; - default: + + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + virBufferAdjustIndent(buf, 8); + if (virDomainHostdevSourceFormat(buf, &def->data.hostdev.def, + flags, true) < 0) { + return -1; + } + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, + buf) < 0) { + return -1; + } + virBufferAdjustIndent(buf, -8); + break; + + + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, buf) < 0) + goto error; + virBufferAdjustIndent(buf, -8); + break; + case VIR_DOMAIN_NET_TYPE_NETWORK: break; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected net type %s"), type); + goto error; } virBufferAdjustIndent(buf, 8); @@ -10610,7 +10703,12 @@ virDomainNetDefFormat(virBufferPtr buf, return -1; } - virBufferAsprintf(buf, " <interface type='%s'>\n", type); + virBufferAsprintf(buf, " <interface type='%s'", type); + if ((def->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) && + def->data.hostdev.def.managed) { + virBufferAddLit(buf, " managed='yes'"); + } + virBufferAddLit(buf, ">\n"); virBufferAsprintf(buf, " <mac address='%02x:%02x:%02x:%02x:%02x:%02x'/>\n", @@ -10629,7 +10727,7 @@ virDomainNetDefFormat(virBufferPtr buf, return -1; virBufferAdjustIndent(buf, -6); if ((flags & VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET) && - (virDomainActualNetDefFormat(buf, def->data.network.actual) < 0)) + (virDomainActualNetDefFormat(buf, def->data.network.actual, flags) < 0)) return -1; break; @@ -10683,6 +10781,19 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, -6); break; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + virBufferAdjustIndent(buf, 6); + if (virDomainHostdevSourceFormat(buf, &def->data.hostdev.def, + flags, true) < 0) { + return -1; + } + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, + buf) < 0) { + return -1; + } + virBufferAdjustIndent(buf, -6); + break; + case VIR_DOMAIN_NET_TYPE_USER: case VIR_DOMAIN_NET_TYPE_LAST: break; @@ -13936,6 +14047,19 @@ virDomainNetGetActualDirectMode(virDomainNetDefPtr iface) return iface->data.network.actual->data.direct.mode; } +virDomainHostdevDefPtr +virDomainNetGetActualHostdev(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) + return &iface->data.hostdev.def; + else if ((iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) && + (iface->data.network.actual->type + == VIR_DOMAIN_NET_TYPE_HOSTDEV)) { + return &iface->data.network.actual->data.hostdev.def; + } + return NULL; +} + virNetDevVPortProfilePtr virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface) { @@ -13944,6 +14068,8 @@ virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface) return iface->data.direct.virtPortProfile; case VIR_DOMAIN_NET_TYPE_BRIDGE: return iface->data.bridge.virtPortProfile; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + return iface->data.hostdev.virtPortProfile; case VIR_DOMAIN_NET_TYPE_NETWORK: if (!iface->data.network.actual) return NULL; @@ -13952,6 +14078,8 @@ virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface) return iface->data.network.actual->data.direct.virtPortProfile; case VIR_DOMAIN_NET_TYPE_BRIDGE: return iface->data.network.actual->data.bridge.virtPortProfile; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + return iface->data.network.actual->data.hostdev.virtPortProfile; default: return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 924c7ec..42c9517 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -681,6 +681,7 @@ enum virDomainNetType { VIR_DOMAIN_NET_TYPE_BRIDGE, VIR_DOMAIN_NET_TYPE_INTERNAL, VIR_DOMAIN_NET_TYPE_DIRECT, + VIR_DOMAIN_NET_TYPE_HOSTDEV, VIR_DOMAIN_NET_TYPE_LAST, }; @@ -731,6 +732,10 @@ struct _virDomainActualNetDef { int mode; /* enum virMacvtapMode from util/macvtap.h */ virNetDevVPortProfilePtr virtPortProfile; } direct; + struct { + virDomainHostdevDef def; + virNetDevVPortProfilePtr virtPortProfile; + } hostdev; } data; virNetDevBandwidthPtr bandwidth; }; @@ -786,6 +791,10 @@ struct _virDomainNetDef { int mode; /* enum virMacvtapMode from util/macvtap.h */ virNetDevVPortProfilePtr virtPortProfile; } direct; + struct { + virDomainHostdevDef def; + virNetDevVPortProfilePtr virtPortProfile; + } hostdev; } data; struct { bool sndbuf_specified; @@ -1921,6 +1930,7 @@ int virDomainNetGetActualType(virDomainNetDefPtr iface); const char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface); const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface); int virDomainNetGetActualDirectMode(virDomainNetDefPtr iface); +virDomainHostdevDefPtr virDomainNetGetActualHostdev(virDomainNetDefPtr iface); virNetDevVPortProfilePtr virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface); virNetDevBandwidthPtr diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dc55f7b..00b085c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -379,6 +379,7 @@ virDomainNetGetActualBandwidth; virDomainNetGetActualBridgeName; virDomainNetGetActualDirectDev; virDomainNetGetActualDirectMode; +virDomainNetGetActualHostdev; virDomainNetGetActualType; virDomainNetGetActualVirtPortProfile; virDomainNetIndexByMac; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dffb6d4..a5a0054 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2606,6 +2606,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_LAST: break; } diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index dbbbfda..0e281ff 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -257,6 +257,11 @@ umlBuildCommandLineNet(virConnectPtr conn, _("direct networking type not supported")); goto error; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + umlReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("hostdev networking type not supported")); + goto error; + case VIR_DOMAIN_NET_TYPE_LAST: break; } diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 81fc0af..b0e1b36 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1955,6 +1955,7 @@ xenFormatSxprNet(virConnectPtr conn, case VIR_DOMAIN_NET_TYPE_MCAST: case VIR_DOMAIN_NET_TYPE_INTERNAL: case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: case VIR_DOMAIN_NET_TYPE_LAST: break; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml new file mode 100644 index 0000000..73cb1d9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219136</memory> + <currentMemory>219136</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <interface type='hostdev' managed='yes'> + <mac address='00:11:22:33:44:55'/> + <source> + <address type='pci' domain='0x0002' bus='0x03' slot='0x07' function='0x1'/> + </source> + <virtualport type='802.1Qbg'> + <parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> + </virtualport> + </interface> + <interface type='hostdev'> + <mac address='11:11:22:33:44:55'/> + <source> + <address type='usb' bus='0' device='2'/> + </source> + </interface> + <interface type='hostdev'> + <mac address='22:11:22:33:44:55'/> + <source> + <vendor id='0x0012'/> + <product id='0x24dd'/> + </source> + </interface> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c1b2b14..9869c79 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -170,6 +170,7 @@ mymain(void) DO_TEST("net-eth"); DO_TEST("net-eth-ifname"); DO_TEST("net-virtio-network-portgroup"); + DO_TEST("net-hostdev"); DO_TEST("sound"); DO_TEST("net-bandwidth"); -- 1.7.7.6
On 02/20/2012 10:10 AM, Laine Stump wrote:
This is the new interface type that sets up a PCI/USB network device to be assigned to the guest with PCI/USB passthrough after initializing some network device-specific things from the config (e.g. MAC address, virtualport profile parameters). Here is an example of the syntax:
<interface type='hostdev' managed='yes'> <source> <address type='pci' domain='0' bus='0' slot='4' function='0'/> </source> <mac address='00:11:22:33:44:55'/> <address type='pci' domain='0' bus='0' slot='7' function='0'/> </interface>
This would assign the PCI card from bus 0 slot 4 function 0 on the host, to bus 0 slot 7 function 0 on the guest, but would first set the MAC address of the card to 00:11:22:33:44:55.
Although it's not expected to be used very much, usb network hostdevs are also supported for completeness.
Even for common things like USB wifi sticks? (Hmm, I've got one of those lying around - might be fun to play with :)
@@ -966,6 +967,12 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def) VIR_FREE(def->data.direct.linkdev); VIR_FREE(def->data.direct.virtPortProfile); break; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + /* currently there is nothing in a virDomainHostdevDef + * that requires freeing. + */ + VIR_FREE(def->data.hostdev.virtPortProfile);
Is that comment still accurate? (Two instances)
@@ -4082,6 +4100,31 @@ virDomainActualNetDefParseXML(xmlNodePtr node, (!(actual->data.direct.virtPortProfile = virNetDevVPortProfileParse(virtPortNode)))) goto error; + } else if (actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt); + virDomainHostdevDefPtr hostdev = &actual->data.hostdev.def; + + hostdev->parent.type = VIR_DOMAIN_DEVICE_NET; + hostdev->parent.data.net = parent; + hostdev->info = &parent->info;
Might be some tweaks here if you take my comments on 5/6 to have hostdev track just a pointer, rather than a full parent struct.
+ /* The helper function expects type to already be found and + * passed in as a string, since it is in a different place in + * NetDef vs HostdevDef. + */ + addrtype = virXPathString("string(./source/address/@type)", ctxt); + if ((!addrtype) && virXPathNode("./source/vendor", ctxt)) + addrtype = strdup("usb"); /* source/vendor implies usb device */
Indentation, and check for OOM.
@@ -4397,6 +4446,27 @@ virDomainNetDefParseXML(virCapsPtr caps,
break;
+ case VIR_DOMAIN_NET_TYPE_HOSTDEV: + hostdev = &def->data.hostdev.def; + hostdev->parent.type = VIR_DOMAIN_DEVICE_NET; + hostdev->parent.data.net = def; + hostdev->info = &def->info; + /* The helper function expects type to already be found and + * passed in as a string, since it is in a different place in + * NetDef vs HostdevDef. + */ + addrtype = virXPathString("string(./source/address/@type)", ctxt); + if ((!addrtype) && virXPathNode("./source/vendor", ctxt)) + addrtype = strdup("usb"); /* source/vendor implies usb device */
Again.
+ case VIR_DOMAIN_NET_TYPE_HOSTDEV: + virBufferAdjustIndent(buf, 8); + if (virDomainHostdevSourceFormat(buf, &def->data.hostdev.def, + flags, true) < 0) { + return -1; + } + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, + buf) < 0) { + return -1; + } + virBufferAdjustIndent(buf, -8); + break; + + + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, buf) < 0) + goto error; + virBufferAdjustIndent(buf, -8);
Oops - duplicated code (two profile prints, and two unindents but only one indent).
+virDomainHostdevDefPtr +virDomainNetGetActualHostdev(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) + return &iface->data.hostdev.def; + else if ((iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) && + (iface->data.network.actual->type + == VIR_DOMAIN_NET_TYPE_HOSTDEV)) {
Style: you have 'if ... else if { ... }' with mismatched bracing. Hmm, since the first if just calls return, you can s/else //, and you've fixed the style problem without having to touch {}. Looking forward to a continuation of this series. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/20/2012 05:40 PM, Eric Blake wrote:
This is the new interface type that sets up a PCI/USB network device to be assigned to the guest with PCI/USB passthrough after initializing some network device-specific things from the config (e.g. MAC address, virtualport profile parameters). Here is an example of the syntax:
<interface type='hostdev' managed='yes'> <source> <address type='pci' domain='0' bus='0' slot='4' function='0'/> </source> <mac address='00:11:22:33:44:55'/> <address type='pci' domain='0' bus='0' slot='7' function='0'/> </interface>
This would assign the PCI card from bus 0 slot 4 function 0 on the host, to bus 0 slot 7 function 0 on the guest, but would first set the MAC address of the card to 00:11:22:33:44:55.
Although it's not expected to be used very much, usb network hostdevs are also supported for completeness. Even for common things like USB wifi sticks? (Hmm, I've got one of
On 02/20/2012 10:10 AM, Laine Stump wrote: those lying around - might be fun to play with :)
Oh sure, usb network devices exist (I've got a >10 year old USB1 wired ethernet adapter hanging off the back of my desktop system just for the fun of it :-), it just seems doubtful anyone using one of those devices would have a need for either 1) setting a difference MAC address than what's in the hardware, or 2) associating it with a vepa/vnlink-capable switch. And if that's the case, it's probably just as well to use a standard <hostdev> entry to assign it to the guest (or use one of the emulated network devices). Mainly I'm just including it for consistency with <hostdev>.
@@ -966,6 +967,12 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def) VIR_FREE(def->data.direct.linkdev); VIR_FREE(def->data.direct.virtPortProfile); break; + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + /* currently there is nothing in a virDomainHostdevDef + * that requires freeing. + */ + VIR_FREE(def->data.hostdev.virtPortProfile); Is that comment still accurate? (Two instances)
Yes, it is accurate. For a non-embedded hostdevdef, there is just an info that needs to be freed. For an embedded hostdevdef (which this is by definition), the info is just a pointer to the parent device object's info, so there is nothing to clear. I guess in terms of future maintenance, this could be problematic though. You've convinced me to add a virDomainHostdefDefClear() function and call it in each of these cases. That way if somebody ever does add something that needs freeing, they'll hopefully do the freeing in the ...Clear() function and we'll be covered. Of course that function should be a part of Patch 1/6, so I'll have to re-spin it...
@@ -4082,6 +4100,31 @@ virDomainActualNetDefParseXML(xmlNodePtr node, (!(actual->data.direct.virtPortProfile = virNetDevVPortProfileParse(virtPortNode)))) goto error; + } else if (actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt); + virDomainHostdevDefPtr hostdev = &actual->data.hostdev.def; + + hostdev->parent.type = VIR_DOMAIN_DEVICE_NET; + hostdev->parent.data.net = parent; + hostdev->info = &parent->info; Might be some tweaks here if you take my comments on 5/6 to have hostdev track just a pointer, rather than a full parent struct.
As I said in my response to the 5/6 review, I really prefer to leave it as it is, since a virDomainDeviceDef really is just a pointer + a single int anyway.
+ /* The helper function expects type to already be found and + * passed in as a string, since it is in a different place in + * NetDef vs HostdevDef. + */ + addrtype = virXPathString("string(./source/address/@type)", ctxt); + if ((!addrtype) && virXPathNode("./source/vendor", ctxt)) + addrtype = strdup("usb"); /* source/vendor implies usb device */ Indentation, and check for OOM.
Yeah, I suppose that would be more informative than just relying on the error log in virDomainHostdevPartsParse() (which would just say that address type is missing). (NB: virXPathString(), which is called here to try and get the type from ./source/address/@type, will log an error message and return NULL if it encounteres OOM, but if the attribute just isn't in the document it will return,... , well, it will also return a NULL in that case too. So although a message is logged, the current operation isn't aborted. Don't know if that's worth doing anything about...)
@@ -4397,6 +4446,27 @@ virDomainNetDefParseXML(virCapsPtr caps,
break;
+ case VIR_DOMAIN_NET_TYPE_HOSTDEV: + hostdev = &def->data.hostdev.def; + hostdev->parent.type = VIR_DOMAIN_DEVICE_NET; + hostdev->parent.data.net = def; + hostdev->info = &def->info; + /* The helper function expects type to already be found and + * passed in as a string, since it is in a different place in + * NetDef vs HostdevDef. + */ + addrtype = virXPathString("string(./source/address/@type)", ctxt); + if ((!addrtype) && virXPathNode("./source/vendor", ctxt)) + addrtype = strdup("usb"); /* source/vendor implies usb device */ Again.
+ case VIR_DOMAIN_NET_TYPE_HOSTDEV: + virBufferAdjustIndent(buf, 8); + if (virDomainHostdevSourceFormat(buf, &def->data.hostdev.def, + flags, true) < 0) { + return -1; + } + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, + buf) < 0) { + return -1; + } + virBufferAdjustIndent(buf, -8); + break; + + + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, buf) < 0) + goto error; + virBufferAdjustIndent(buf, -8); Oops - duplicated code (two profile prints, and two unindents but only one indent).
Strange. I really would like to attribute that to git rebase doing something odd, as I'd like to believe I didn't actually write it that way, but I might have accidentally hit the past button twice or something.
+virDomainHostdevDefPtr +virDomainNetGetActualHostdev(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) + return &iface->data.hostdev.def; + else if ((iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) && + (iface->data.network.actual->type + == VIR_DOMAIN_NET_TYPE_HOSTDEV)) { Style: you have 'if ... else if { ... }' with mismatched bracing.
Hmm, since the first if just calls return, you can s/else //, and you've fixed the style problem without having to touch {}.
Yep. That entire function was a last minute addition, and I didn't spend as much time worrying over it as normal...
participants (2)
-
Eric Blake -
Laine Stump