On 29.06.2012 17:02, Viktor Mihajlovski wrote:
> This is in preparation of the enablement of s390 guests with virtio devices.
>
> The assignment of device addresses happens in different places, i.e. the
> qemu driver and process modules as well as in the unit tests in slightly
> different flavors. Currently, these are PPC spapr-vio and PCI
> devices, virtio-s390 (not PCI based) will follow.
>
> By optionally passing to qemuDomainAssignAddresses the domain
> object and the capabilities it is now possible to call the function
> from most of the places (except for hotplug) where address assignment
> is done.
>
> Signed-off-by: Viktor Mihajlovski <mihajlov@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_command.c | 41 ++++++++++++++++++++++++++++++++---------
> src/qemu/qemu_command.h | 6 ++++--
> src/qemu/qemu_driver.c | 14 +++++++-------
> src/qemu/qemu_process.c | 42 ++++--------------------------------------
> 4 files changed, 47 insertions(+), 56 deletions(-)
Nice cleanup.
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6549f57..5edf915 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -942,16 +942,22 @@ cleanup:
>
>
> int
> -qemuDomainAssignPCIAddresses(virDomainDefPtr def)
> +qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
> + virDomainObjPtr obj)
Even though this is still less than 80 characters, I think we could have each argument per separate line.
> {
> int ret = -1;
> - virBitmapPtr qemuCaps = NULL;
> + virBitmapPtr localCaps = NULL;
> qemuDomainPCIAddressSetPtr addrs = NULL;
> + qemuDomainObjPrivatePtr priv = NULL;
>
> - if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch,
> - NULL,
> - &qemuCaps) < 0)
> - goto cleanup;
> + if (!qemuCaps) {
> + /* need to get information from real environment */
> + if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch,
> + NULL,
> + &localCaps) < 0)
> + goto cleanup;
> + qemuCaps = localCaps;
> + }
>
> if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> if (!(addrs = qemuDomainPCIAddressSetCreate(def)))
> @@ -961,16 +967,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def)
> goto cleanup;
> }
>
> + if (obj && obj->privateData) {
> + priv = obj->privateData;
> + if (addrs) {
> + /* if this is the live domain object, we persist the PCI addresses*/
> + if (priv->pciaddrs) {
> + qemuDomainPCIAddressSetFree(priv->pciaddrs);
> + priv->pciaddrs = NULL;
> + }
qemuDomainPCIAddressSetFree() handles passed NULL, so this check is redundant.
> + priv->persistentAddrs = 1;
> + priv->pciaddrs = addrs;
> + addrs = NULL;
> + } else {
> + priv->persistentAddrs = 0;
> + }
> + }
> +
> ret = 0;
>
> cleanup:
> - qemuCapsFree(qemuCaps);
> + qemuCapsFree(localCaps);
> qemuDomainPCIAddressSetFree(addrs);
>
> return ret;
> }
>
> -int qemuDomainAssignAddresses(virDomainDefPtr def)
> +int qemuDomainAssignAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
> + virDomainObjPtr obj)
Again. I'd prefer to have each argument on a separate line...
> {
> int rc;
>
> @@ -978,7 +1001,7 @@ int qemuDomainAssignAddresses(virDomainDefPtr def)
> if (rc)
> return rc;
>
> - return qemuDomainAssignPCIAddresses(def);
> + return qemuDomainAssignPCIAddresses(def, qemuCaps, obj);
> }
>
> static void
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 1eafeb3..dd104d6 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -175,10 +175,12 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps,
> virDomainChrSourceDefPtr *monConfig,
> bool *monJSON);
>
> -int qemuDomainAssignAddresses(virDomainDefPtr def);
> +int qemuDomainAssignAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
> + virDomainObjPtr);
> int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def);
>
> -int qemuDomainAssignPCIAddresses(virDomainDefPtr def);
> +int qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
> + virDomainObjPtr obj);
> qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def);
... and same here.
> int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs,
> int slot, int function);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2f93404..ef9983c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1404,7 +1404,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
> if (qemudCanonicalizeMachine(driver, def) < 0)
> goto cleanup;
>
> - if (qemuDomainAssignAddresses(def) < 0)
> + if (qemuDomainAssignAddresses(def, NULL, NULL) < 0)
> goto cleanup;
>
> if (!(vm = virDomainAssignDef(driver->caps,
> @@ -5070,7 +5070,7 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) {
> if (qemudCanonicalizeMachine(driver, def) < 0)
> goto cleanup;
>
> - if (qemuDomainAssignAddresses(def) < 0)
> + if (qemuDomainAssignAddresses(def, NULL, NULL) < 0)
> goto cleanup;
>
> if (!(vm = virDomainAssignDef(driver->caps,
> @@ -5548,7 +5548,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
> if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
> if (virDomainDefAddImplicitControllers(vmdef) < 0)
> return -1;
> - if (qemuDomainAssignAddresses(vmdef) < 0)
> + if (qemuDomainAssignAddresses(vmdef, NULL, NULL) < 0)
> return -1;
> break;
>
> @@ -5566,7 +5566,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
> return -1;
> }
> dev->data.net = NULL;
> - if (qemuDomainAssignAddresses(vmdef) < 0)
> + if (qemuDomainAssignAddresses(vmdef, NULL, NULL) < 0)
> return -1;
> break;
>
> @@ -5582,7 +5582,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
> return -1;
> }
> dev->data.hostdev = NULL;
> - if (qemuDomainAssignAddresses(vmdef) < 0)
> + if (qemuDomainAssignAddresses(vmdef, NULL, NULL) < 0)
> return -1;
> break;
>
> @@ -5736,7 +5736,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
> vmdef->nets[pos] = net;
> dev->data.net = NULL;
>
> - if (qemuDomainAssignAddresses(vmdef) < 0)
> + if (qemuDomainAssignAddresses(vmdef, NULL, NULL) < 0)
> return -1;
> break;
>
> @@ -11734,7 +11734,7 @@ static virDomainPtr qemuDomainAttach(virConnectPtr conn,
> if (qemudCanonicalizeMachine(driver, def) < 0)
> goto cleanup;
>
> - if (qemuDomainAssignAddresses(def) < 0)
> + if (qemuDomainAssignAddresses(def, NULL, NULL) < 0)
> goto cleanup;
>
> if (!(vm = virDomainAssignDef(driver->caps,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c5140c3..bf32487 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3090,13 +3090,8 @@ qemuProcessReconnect(void *opaque)
> goto endjob;
> }
>
> - if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> - priv->persistentAddrs = 1;
> -
> - if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(obj->def)) ||
> - qemuAssignDevicePCISlots(obj->def, priv->pciaddrs) < 0)
> - goto error;
> - }
> + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE))
> + qemuDomainAssignAddresses(obj->def, priv->qemuCaps, obj);
>
> if (virSecurityManagerReserveLabel(driver->securityManager, obj->def, obj->pid) < 0)
> goto error;
> @@ -3560,22 +3555,7 @@ int qemuProcessStart(virConnectPtr conn,
> */
> if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> VIR_DEBUG("Assigning domain PCI addresses");
> - /* Populate cache with current addresses */
> - if (priv->pciaddrs) {
> - qemuDomainPCIAddressSetFree(priv->pciaddrs);
> - priv->pciaddrs = NULL;
> - }
> - if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(vm->def)))
> - goto cleanup;
> -
> -
> - /* Assign any remaining addresses */
> - if (qemuAssignDevicePCISlots(vm->def, priv->pciaddrs) < 0)
> - goto cleanup;
> -
> - priv->persistentAddrs = 1;
> - } else {
> - priv->persistentAddrs = 0;
> + qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm);
> }
>
> VIR_DEBUG("Building emulator command line");
> @@ -4257,21 +4237,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
> */
> if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> VIR_DEBUG("Assigning domain PCI addresses");
> - /* Populate cache with current addresses */
> - if (priv->pciaddrs) {
> - qemuDomainPCIAddressSetFree(priv->pciaddrs);
> - priv->pciaddrs = NULL;
> - }
> - if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(vm->def)))
> - goto cleanup;
> -
> - /* Assign any remaining addresses */
> - if (qemuAssignDevicePCISlots(vm->def, priv->pciaddrs) < 0)
> - goto cleanup;
> -
> - priv->persistentAddrs = 1;
> - } else {
> - priv->persistentAddrs = 0;
> + qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm);
> }
>
> if ((timestamp = virTimeStringNow()) == NULL) {
>
ACK with this squashed in:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a6fc9ca..bcc2192 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -942,7 +942,8 @@ cleanup:
int
-qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
+qemuDomainAssignPCIAddresses(virDomainDefPtr def,
+ virBitmapPtr qemuCaps,
virDomainObjPtr obj)
{
int ret = -1;
@@ -971,10 +972,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
priv = obj->privateData;
if (addrs) {
/* if this is the live domain object, we persist the PCI addresses*/
- if (priv->pciaddrs) {
- qemuDomainPCIAddressSetFree(priv->pciaddrs);
- priv->pciaddrs = NULL;
- }
+ qemuDomainPCIAddressSetFree(priv->pciaddrs);
priv->persistentAddrs = 1;
priv->pciaddrs = addrs;
addrs = NULL;
@@ -992,7 +990,8 @@ cleanup:
return ret;
}
-int qemuDomainAssignAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
+int qemuDomainAssignAddresses(virDomainDefPtr def,
+ virBitmapPtr qemuCaps,
virDomainObjPtr obj)
{
int rc;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index dd104d6..ddf426f 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -175,11 +175,13 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps,
virDomainChrSourceDefPtr *monConfig,
bool *monJSON);
-int qemuDomainAssignAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
+int qemuDomainAssignAddresses(virDomainDefPtr def,
+ virBitmapPtr qemuCaps,
virDomainObjPtr);
int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def);
-int qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
+int qemuDomainAssignPCIAddresses(virDomainDefPtr def,
+ virBitmapPtr qemuCaps,
virDomainObjPtr obj);
qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def);
int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs,
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list