11:10 a.m.
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.
11:10 a.m.
New subject: [libvirt] [PATCH 1/6] conf: make hostdev info a separate object
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
12:48 p.m.
New subject: [libvirt] [PATCH 1/6] conf: make hostdev info a separate object
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:46 p.m.
New subject: [libvirt] [PATCH 1/6] conf: make hostdev info a separate object
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.
3:43 p.m.
New subject: [libvirt] [PATCH 1/6] conf: make hostdev info a separate object
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:14 p.m.
New subject: [libvirt] [PATCH 1/6] conf: make hostdev info a separate object
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
11:10 a.m.
New subject: [libvirt] [PATCH 2/6] conf: relocate virDomainDeviceDef and virDomainHostdevDef
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
12:55 p.m.
New subject: [libvirt] [PATCH 2/6] conf: relocate virDomainDeviceDef and virDomainHostdevDef
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:59 p.m.
New subject: [libvirt] [PATCH 2/6] conf: relocate virDomainDeviceDef and virDomainHostdevDef
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".
11:10 a.m.
New subject: [libvirt] [PATCH 3/6] conf: reorder static functions in domain_conf.c
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
1:06 p.m.
New subject: [libvirt] [PATCH 3/6] conf: reorder static functions in domain_conf.c
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:10 a.m.
New subject: [libvirt] [PATCH 4/6] conf: HostdevDef parse/format helper functions
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
3:56 p.m.
New subject: [libvirt] [PATCH 4/6] conf: HostdevDef parse/format helper functions
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:10 a.m.
New subject: [libvirt] [PATCH 5/6] conf: give each hostdevdef a parent pointer
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
4:02 p.m.
New subject: [libvirt] [PATCH 5/6] conf: give each hostdevdef a parent pointer
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:22 a.m.
New subject: [libvirt] [PATCH 5/6] conf: give each hostdevdef a parent pointer
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.
11:10 a.m.
New subject: [libvirt] [PATCH 6/6] conf: parse/format type='hostdev' network interfaces
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
4:40 p.m.
New subject: [libvirt] [PATCH 6/6] conf: parse/format type='hostdev' network interfaces
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:48 p.m.
New subject: [libvirt] [PATCH 6/6] conf: parse/format type='hostdev' network interfaces
On 02/20/2012 05:40 PM, Eric Blake wrote:
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 :)
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...
4693
days inactive
4694
days old
18 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump