[libvirt] [PATCH v2 00/14] qemu_hotplug: refactor device detach functions to fix one serious and other minor bugs
6:24 p.m.
V1: https://www.redhat.com/archives/libvir-list/2019-March/msg01455.html
I pushed patches 01-12, since they were acked, and pushing them leaves
the code in no worse state than it was before (all tests still pass).
There are 4 new patches in V2, created as a result of Peter asking for
patches to be split, or for further refactoring.
Still to be done: Peter pointed out that qemuDomainUpdateDeviceLlist()
really shouldn't be called until after qemu has sent the
DEVICE_DELETED event. That is on my list of followups, but the patches
here are still a valid improvement even without that (After his
comment, I noticed that we are also calling virDomainSaveStatus too
soon - it also shouldn't be called until after DEVICE_DELETED).
Laine Stump (14):
qemu_hotplug: move qemuDomainDetachDeviceLive() to qemu_hotplug.c
qemu_hotplug: remove extra function in middle of DetachController call
chain
qemu_hotplug: pull qemuDomainUpdateDeviceList out of
qemuDomainDetachDeviceLive
test: replace calls to individual detach functions with one call to
main detach
qemu_hotplug: make Detach functions called only from qemu_hotplug.c
static
qemu_hotplug: rename dev to match in qemuDomainDetachDeviceLive
qemu_hotplug: separate Chr|Lease from other devices in DetachDevice
switch
qemu_hotplug: standardize the names/args/calling of
qemuDomainDetach*()
qemu_hotplug: rename Chr and Lease Detach functions
qemu_hotplug: new function qemuDomainRemoveAuditDevice()
qemu_hotplug: audit *all* auditable device types in
qemuDomainRemoveAuditDevice
qemu_hotplug: consolidate all common detach code in
qemuDomainDetachDeviceLive
qemu_hotplug: delay sending DEVICE_REMOVED event until after *all*
teardown
qemu_hotplug: don't shutdown net device until the guest has released
it
src/qemu/qemu_driver.c | 109 +----
src/qemu/qemu_hotplug.c | 919 +++++++++++++++++++---------------------
src/qemu/qemu_hotplug.h | 56 +--
tests/qemuhotplugtest.c | 8 +-
4 files changed, 465 insertions(+), 627 deletions(-)
--
2.20.1
6:24 p.m.
New subject: [libvirt] [PATCH v2 01/14] qemu_hotplug: move qemuDomainDetachDeviceLive() to qemu_hotplug.c
This function is going to take on some of the functionality of its
subordinate functions, which all live in qemu_hotplug.c.
qemuDomainDetachDeviceControllerLive() is only called from
qemuDomainDetachDeviceLive() (and will soon be merged into
qemuDomainDetachControllerDevice(), which is in qemu_hotplug.c), so
it is also moved.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1: take out changes of functions from global to static, move them to a
separage patch (now in Patch 05/14)
src/qemu/qemu_driver.c | 95 ----------------------------------------
src/qemu/qemu_hotplug.c | 97 +++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_hotplug.h | 5 +++
3 files changed, 102 insertions(+), 95 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b27c6ce98e..6f5a4224c5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8001,101 +8001,6 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
return ret;
}
-static int
-qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async)
-{
- virDomainControllerDefPtr cont = dev->data.controller;
- int ret = -1;
-
- switch (cont->type) {
- case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
- ret = qemuDomainDetachControllerDevice(driver, vm, dev, async);
- break;
- default :
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
- _("'%s' controller cannot be hot unplugged."),
- virDomainControllerTypeToString(cont->type));
- }
- return ret;
-}
-
-static int
-qemuDomainDetachDeviceLive(virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- virQEMUDriverPtr driver,
- bool async)
-{
- int ret = -1;
-
- switch ((virDomainDeviceType)dev->type) {
- case VIR_DOMAIN_DEVICE_DISK:
- ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async);
- break;
- case VIR_DOMAIN_DEVICE_CONTROLLER:
- ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev, async);
- break;
- case VIR_DOMAIN_DEVICE_LEASE:
- ret = qemuDomainDetachLease(driver, vm, dev->data.lease);
- break;
- case VIR_DOMAIN_DEVICE_NET:
- ret = qemuDomainDetachNetDevice(driver, vm, dev, async);
- break;
- case VIR_DOMAIN_DEVICE_HOSTDEV:
- ret = qemuDomainDetachHostDevice(driver, vm, dev, async);
- break;
- case VIR_DOMAIN_DEVICE_CHR:
- ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr, async);
- break;
- case VIR_DOMAIN_DEVICE_RNG:
- ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng, async);
- break;
- case VIR_DOMAIN_DEVICE_MEMORY:
- ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory, async);
- break;
- case VIR_DOMAIN_DEVICE_SHMEM:
- ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem, async);
- break;
- case VIR_DOMAIN_DEVICE_WATCHDOG:
- ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog, async);
- break;
- case VIR_DOMAIN_DEVICE_INPUT:
- ret = qemuDomainDetachInputDevice(vm, dev->data.input, async);
- break;
- case VIR_DOMAIN_DEVICE_REDIRDEV:
- ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev, async);
- break;
-
- case VIR_DOMAIN_DEVICE_VSOCK:
- ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async);
- break;
-
- case VIR_DOMAIN_DEVICE_FS:
- case VIR_DOMAIN_DEVICE_SOUND:
- case VIR_DOMAIN_DEVICE_VIDEO:
- case VIR_DOMAIN_DEVICE_GRAPHICS:
- case VIR_DOMAIN_DEVICE_HUB:
- case VIR_DOMAIN_DEVICE_SMARTCARD:
- case VIR_DOMAIN_DEVICE_MEMBALLOON:
- case VIR_DOMAIN_DEVICE_NVRAM:
- case VIR_DOMAIN_DEVICE_NONE:
- case VIR_DOMAIN_DEVICE_TPM:
- case VIR_DOMAIN_DEVICE_PANIC:
- case VIR_DOMAIN_DEVICE_IOMMU:
- case VIR_DOMAIN_DEVICE_LAST:
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
- _("live detach of device '%s' is not
supported"),
- virDomainDeviceTypeToString(dev->type));
- break;
- }
-
- if (ret == 0)
- ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
-
- return ret;
-}
static int
qemuDomainChangeDiskLive(virDomainObjPtr vm,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c1d2d00016..7be0f341b7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6190,6 +6190,103 @@ qemuDomainDetachLease(virQEMUDriverPtr driver,
}
+static int
+qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ bool async)
+{
+ virDomainControllerDefPtr cont = dev->data.controller;
+ int ret = -1;
+
+ switch (cont->type) {
+ case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
+ ret = qemuDomainDetachControllerDevice(driver, vm, dev, async);
+ break;
+ default :
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("'%s' controller cannot be hot unplugged."),
+ virDomainControllerTypeToString(cont->type));
+ }
+ return ret;
+}
+
+int
+qemuDomainDetachDeviceLive(virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ virQEMUDriverPtr driver,
+ bool async)
+{
+ int ret = -1;
+
+ switch ((virDomainDeviceType)dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async);
+ break;
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev, async);
+ break;
+ case VIR_DOMAIN_DEVICE_LEASE:
+ ret = qemuDomainDetachLease(driver, vm, dev->data.lease);
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ ret = qemuDomainDetachNetDevice(driver, vm, dev, async);
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ ret = qemuDomainDetachHostDevice(driver, vm, dev, async);
+ break;
+ case VIR_DOMAIN_DEVICE_CHR:
+ ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr, async);
+ break;
+ case VIR_DOMAIN_DEVICE_RNG:
+ ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng, async);
+ break;
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory, async);
+ break;
+ case VIR_DOMAIN_DEVICE_SHMEM:
+ ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem, async);
+ break;
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog, async);
+ break;
+ case VIR_DOMAIN_DEVICE_INPUT:
+ ret = qemuDomainDetachInputDevice(vm, dev->data.input, async);
+ break;
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev, async);
+ break;
+
+ case VIR_DOMAIN_DEVICE_VSOCK:
+ ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async);
+ break;
+
+ case VIR_DOMAIN_DEVICE_FS:
+ case VIR_DOMAIN_DEVICE_SOUND:
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ case VIR_DOMAIN_DEVICE_GRAPHICS:
+ case VIR_DOMAIN_DEVICE_HUB:
+ case VIR_DOMAIN_DEVICE_SMARTCARD:
+ case VIR_DOMAIN_DEVICE_MEMBALLOON:
+ case VIR_DOMAIN_DEVICE_NVRAM:
+ case VIR_DOMAIN_DEVICE_NONE:
+ case VIR_DOMAIN_DEVICE_TPM:
+ case VIR_DOMAIN_DEVICE_PANIC:
+ case VIR_DOMAIN_DEVICE_IOMMU:
+ case VIR_DOMAIN_DEVICE_LAST:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("live detach of device '%s' is not
supported"),
+ virDomainDeviceTypeToString(dev->type));
+ break;
+ }
+
+ if (ret == 0)
+ ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
+
+ return ret;
+}
+
+
static int
qemuDomainRemoveVcpu(virQEMUDriverPtr driver,
virDomainObjPtr vm,
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 7ac03b7810..10593cf83c 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -158,6 +158,11 @@ int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
virDomainRNGDefPtr rng,
bool async);
+int qemuDomainDetachDeviceLive(virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ virQEMUDriverPtr driver,
+ bool async);
+
void qemuDomainRemoveVcpuAlias(virQEMUDriverPtr driver,
virDomainObjPtr vm,
const char *alias);
--
2.20.1
8:36 a.m.
New subject: [libvirt] [PATCH v2 01/14] qemu_hotplug: move qemuDomainDetachDeviceLive() to qemu_hotplug.c
On Mon, Mar 25, 2019 at 13:24:23 -0400, Laine Stump wrote:
This function is going to take on some of the functionality of its
subordinate functions, which all live in qemu_hotplug.c.
qemuDomainDetachDeviceControllerLive() is only called from
qemuDomainDetachDeviceLive() (and will soon be merged into
qemuDomainDetachControllerDevice(), which is in qemu_hotplug.c), so
it is also moved.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 02/14] qemu_hotplug: remove extra function in middle of DetachController call chain
qemuDomainDetachDeviceControllerLive() just checks if the controller
type is SCSI, and then either returns failure, or calls
qemuDomainDetachControllerDevice().
Instead, lets just check for type != SCSI at the top of the latter
function, and call it directly.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1: remove unrelated code movement, per Peter's request.
src/qemu/qemu_hotplug.c | 30 ++++++++----------------------
1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7be0f341b7..666657758e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5548,6 +5548,13 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
int idx, ret = -1;
virDomainControllerDefPtr detach = NULL;
+ if (dev->data.controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("'%s' controller cannot be hot unplugged."),
+
virDomainControllerTypeToString(dev->data.controller->type));
+ return -1;
+ }
+
if ((idx = virDomainControllerFind(vm->def,
dev->data.controller->type,
dev->data.controller->idx)) < 0) {
@@ -6190,27 +6197,6 @@ qemuDomainDetachLease(virQEMUDriverPtr driver,
}
-static int
-qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async)
-{
- virDomainControllerDefPtr cont = dev->data.controller;
- int ret = -1;
-
- switch (cont->type) {
- case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
- ret = qemuDomainDetachControllerDevice(driver, vm, dev, async);
- break;
- default :
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
- _("'%s' controller cannot be hot unplugged."),
- virDomainControllerTypeToString(cont->type));
- }
- return ret;
-}
-
int
qemuDomainDetachDeviceLive(virDomainObjPtr vm,
virDomainDeviceDefPtr dev,
@@ -6224,7 +6210,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async);
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev, async);
+ ret = qemuDomainDetachControllerDevice(driver, vm, dev, async);
break;
case VIR_DOMAIN_DEVICE_LEASE:
ret = qemuDomainDetachLease(driver, vm, dev->data.lease);
--
2.20.1
8:37 a.m.
New subject: [libvirt] [PATCH v2 02/14] qemu_hotplug: remove extra function in middle of DetachController call chain
On Mon, Mar 25, 2019 at 13:24:24 -0400, Laine Stump wrote:
qemuDomainDetachDeviceControllerLive() just checks if the controller
type is SCSI, and then either returns failure, or calls
qemuDomainDetachControllerDevice().
Instead, lets just check for type != SCSI at the top of the latter
function, and call it directly.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 03/14] qemu_hotplug: pull qemuDomainUpdateDeviceList out of qemuDomainDetachDeviceLive
qemuDomainDetachDeviceLive() is called from two places in
qemu_driver.c, and qemuDomainUpdateDeviceList() is called from the
end of qemuDomainDetachDeviceLive(), which is now in qemu_hotplug.c
This patch replaces the single call to qemuDomainUpdateDeviceList()
with two calls to it immediately after return from
qemuDomainDetachDeviceLive(). This is only done if the return from
that function is exactly 0, in order to exactly preserve previous
behavior.
Removing that one call from qemuDomainDetachDeviceList() will permit
us to call it from the test driver hotplug test, replacing the
separate calls to qemuDomainDetachDeviceDiskLive(),
qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and
qemuDomainDetachWatchdog(). We want to do this so that part of the
common functionality of those three functions (and the rest of the
device-specific Detach functions) can be pulled up into
qemuDomainDetachDeviceLive() without breaking the test. (This is done
in the next patch).
NB: Almost certainly this is "not the best place" to call
qemuDomainUpdateDeviceList() (actually, it is provably the *wrong*
place), since it's purpose is to retrieve an "up to date" list of
aliases for all devices from qemu, and if the guest OS hasn't yet
processed the detach request, the now-being-removed device may still
be on that list. It would arguably be better to instead call
qemuDomainUpdateDevicesList() later during the response to the
DEVICE_DELETED event for the device. But removing the call from the
current point in the detach could have some unforeseen ill effect due
to changed timing, so the change to move it into
qemuDomainRemove*Device() will be done in a separate patch (in order
to make it easily revertible in case it causes a regression).
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1:
* Added the big explanation above about why I'm not completely
changing behavior by waiting unt after DEVICE_DELETED to call
qemuDomainUpdateDeviceList()
* Only call qemuDomainUpdateDeviceList() if the Detach function
returned *exactly* 0, since there are cases where it could return >
0, and the previous behavior was to not call it in those cases.
src/qemu/qemu_driver.c | 14 ++++++++++++--
src/qemu/qemu_hotplug.c | 3 ---
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6f5a4224c5..51b434f0d2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8927,8 +8927,14 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (qemuDomainDetachDeviceLive(vm, dev_copy, driver, false) < 0)
+ int rc;
+
+ if ((rc = qemuDomainDetachDeviceLive(vm, dev_copy, driver, false)) < 0)
+ goto cleanup;
+
+ if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm,
QEMU_ASYNC_JOB_NONE) < 0)
goto cleanup;
+
/*
* update domain status forcibly because the domain status may be
* changed even if we failed to attach the device. For example,
@@ -9005,11 +9011,15 @@ qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver,
if (def) {
virDomainDeviceDef dev;
+ int rc;
if (virDomainDefFindDevice(def, alias, &dev, true) < 0)
goto cleanup;
- if (qemuDomainDetachDeviceLive(vm, &dev, driver, true) < 0)
+ if ((rc = qemuDomainDetachDeviceLive(vm, &dev, driver, true)) < 0)
+ goto cleanup;
+
+ if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm,
QEMU_ASYNC_JOB_NONE) < 0)
goto cleanup;
}
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 666657758e..b435ea99ac 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6266,9 +6266,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
break;
}
- if (ret == 0)
- ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
-
return ret;
}
--
2.20.1
8:45 a.m.
New subject: [libvirt] [PATCH v2 03/14] qemu_hotplug: pull qemuDomainUpdateDeviceList out of qemuDomainDetachDeviceLive
On Mon, Mar 25, 2019 at 13:24:25 -0400, Laine Stump wrote:
qemuDomainDetachDeviceLive() is called from two places in
qemu_driver.c, and qemuDomainUpdateDeviceList() is called from the
end of qemuDomainDetachDeviceLive(), which is now in qemu_hotplug.c
This patch replaces the single call to qemuDomainUpdateDeviceList()
with two calls to it immediately after return from
qemuDomainDetachDeviceLive(). This is only done if the return from
that function is exactly 0, in order to exactly preserve previous
behavior.
Removing that one call from qemuDomainDetachDeviceList() will permit
us to call it from the test driver hotplug test, replacing the
separate calls to qemuDomainDetachDeviceDiskLive(),
qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and
qemuDomainDetachWatchdog(). We want to do this so that part of the
common functionality of those three functions (and the rest of the
device-specific Detach functions) can be pulled up into
qemuDomainDetachDeviceLive() without breaking the test. (This is done
in the next patch).
NB: Almost certainly this is "not the best place" to call
qemuDomainUpdateDeviceList() (actually, it is provably the *wrong*
place), since it's purpose is to retrieve an "up to date" list of
aliases for all devices from qemu, and if the guest OS hasn't yet
processed the detach request, the now-being-removed device may still
be on that list. It would arguably be better to instead call
qemuDomainUpdateDevicesList() later during the response to the
DEVICE_DELETED event for the device. But removing the call from the
current point in the detach could have some unforeseen ill effect due
to changed timing, so the change to move it into
qemuDomainRemove*Device() will be done in a separate patch (in order
to make it easily revertible in case it causes a regression).
Actually it probably would cause a regression. It's called only when the
device is removed. That function will require some overhauling.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1:
* Added the big explanation above about why I'm not completely
changing behavior by waiting unt after DEVICE_DELETED to call
qemuDomainUpdateDeviceList()
* Only call qemuDomainUpdateDeviceList() if the Detach function
returned *exactly* 0, since there are cases where it could return >
0, and the previous behavior was to not call it in those cases.
ACK
3:04 p.m.
New subject: [libvirt] [PATCH v2 03/14] qemu_hotplug: pull qemuDomainUpdateDeviceList out of qemuDomainDetachDeviceLive
On 3/26/19 3:45 AM, Peter Krempa wrote:
On Mon, Mar 25, 2019 at 13:24:25 -0400, Laine Stump wrote:
> qemuDomainDetachDeviceLive() is called from two places in
> qemu_driver.c, and qemuDomainUpdateDeviceList() is called from the
> end of qemuDomainDetachDeviceLive(), which is now in qemu_hotplug.c
>
> This patch replaces the single call to qemuDomainUpdateDeviceList()
> with two calls to it immediately after return from
> qemuDomainDetachDeviceLive(). This is only done if the return from
> that function is exactly 0, in order to exactly preserve previous
> behavior.
>
> Removing that one call from qemuDomainDetachDeviceList() will permit
> us to call it from the test driver hotplug test, replacing the
> separate calls to qemuDomainDetachDeviceDiskLive(),
> qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and
> qemuDomainDetachWatchdog(). We want to do this so that part of the
> common functionality of those three functions (and the rest of the
> device-specific Detach functions) can be pulled up into
> qemuDomainDetachDeviceLive() without breaking the test. (This is done
> in the next patch).
>
> NB: Almost certainly this is "not the best place" to call
> qemuDomainUpdateDeviceList() (actually, it is provably the *wrong*
> place), since it's purpose is to retrieve an "up to date" list of
> aliases for all devices from qemu, and if the guest OS hasn't yet
> processed the detach request, the now-being-removed device may still
> be on that list. It would arguably be better to instead call
> qemuDomainUpdateDevicesList() later during the response to the
> DEVICE_DELETED event for the device. But removing the call from the
> current point in the detach could have some unforeseen ill effect due
> to changed timing, so the change to move it into
> qemuDomainRemove*Device() will be done in a separate patch (in order
> to make it easily revertible in case it causes a regression).
Actually it probably would cause a regression. It's called only when the
device is removed. That function will require some overhauling.
Yeah, I started to look at it and decided it was too complicated to try
and pull together in a few minutes while I was rushed, so I just wrote
it on my to-do list.
> Signed-off-by: Laine Stump <laine(a)laine.org>
> ---
>
> Change from V1:
>
> * Added the big explanation above about why I'm not completely
> changing behavior by waiting unt after DEVICE_DELETED to call
> qemuDomainUpdateDeviceList()
>
> * Only call qemuDomainUpdateDeviceList() if the Detach function
> returned *exactly* 0, since there are cases where it could return >
> 0, and the previous behavior was to not call it in those cases.
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 04/14] test: replace calls to individual detach functions with one call to main detach
The individual qemuDomainDetach*Device() functions will soon be "less
functional", since some of the code that is duplicated in 10 of the 12
detach functions is going to be moved into the common
qemuDomainDetachDeviceLive(), which calls them all.
qemuhotplugtest.c is the only place any of these individual functions
is called other than qemuDomainDetachDeviceLive() itself. Fortunately,
qemuDomainDetachDeviceLive() provides exactly the functionality needed
by the test driver (except that it supports detach of more device
types than the test driver has tests for).
This patch replaces the calls to
qemuDomainDetach(Chr|Shmen|Watchdog|Disk)Device with a single call to
the higher level function, allowing us to shift functionality between
the lower level functions without breaking the tests.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1: remove chunks that changed now-uncalled
qemuDomainDetach*() functions to static. That is done in new Patch
05/14.
tests/qemuhotplugtest.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 1491c214d0..3eb97d886a 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -147,16 +147,10 @@ testQemuHotplugDetach(virDomainObjPtr vm,
switch (dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
- ret = qemuDomainDetachDeviceDiskLive(&driver, vm, dev, async);
- break;
case VIR_DOMAIN_DEVICE_CHR:
- ret = qemuDomainDetachChrDevice(&driver, vm, dev->data.chr, async);
- break;
case VIR_DOMAIN_DEVICE_SHMEM:
- ret = qemuDomainDetachShmemDevice(&driver, vm, dev->data.shmem, async);
- break;
case VIR_DOMAIN_DEVICE_WATCHDOG:
- ret = qemuDomainDetachWatchdog(&driver, vm, dev->data.watchdog, async);
+ ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async);
break;
default:
VIR_TEST_VERBOSE("device type '%s' cannot be detached\n",
--
2.20.1
8:47 a.m.
New subject: [libvirt] [PATCH v2 04/14] test: replace calls to individual detach functions with one call to main detach
On Mon, Mar 25, 2019 at 13:24:26 -0400, Laine Stump wrote:
The individual qemuDomainDetach*Device() functions will soon be
"less
functional", since some of the code that is duplicated in 10 of the 12
detach functions is going to be moved into the common
qemuDomainDetachDeviceLive(), which calls them all.
qemuhotplugtest.c is the only place any of these individual functions
is called other than qemuDomainDetachDeviceLive() itself. Fortunately,
qemuDomainDetachDeviceLive() provides exactly the functionality needed
by the test driver (except that it supports detach of more device
types than the test driver has tests for).
This patch replaces the calls to
qemuDomainDetach(Chr|Shmen|Watchdog|Disk)Device with a single call to
the higher level function, allowing us to shift functionality between
the lower level functions without breaking the tests.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 05/14] qemu_hotplug: make Detach functions called only from qemu_hotplug.c static
These are no longer called from qemu_driver.c, since the function that
called them (qemuDomainDetachDeviceLive()) has been moved to
qemu_hotplug.c, and they are no longer called from testqemuhotplug.c
because it now just called qemuDomainDetachDeviceLive() instead of all
the subordinate functions.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH IN V2
src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++------------------
src/qemu/qemu_hotplug.h | 51 -----------------------------------------
2 files changed, 25 insertions(+), 73 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b435ea99ac..50bfd5f4e6 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5389,7 +5389,7 @@ qemuFindDisk(virDomainDefPtr def, const char *dst)
return -1;
}
-int
+static int
qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev,
@@ -5540,10 +5540,11 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm,
}
}
-int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async)
+static int
+qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ bool async)
{
int idx, ret = -1;
virDomainControllerDefPtr detach = NULL;
@@ -5601,10 +5602,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
/* search for a hostdev matching dev and detach it */
-int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async)
+static int
+qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDeviceDefPtr dev,
+ bool async)
{
virDomainHostdevDefPtr hostdev = dev->data.hostdev;
virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys;
@@ -5716,7 +5718,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
}
-int
+static int
qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainShmemDefPtr dev,
@@ -5770,7 +5772,7 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
}
-int
+static int
qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainWatchdogDefPtr dev,
@@ -5825,7 +5827,7 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
}
-int
+static int
qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainRedirdevDefPtr dev,
@@ -5869,7 +5871,7 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
}
-int
+static int
qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev,
@@ -5935,10 +5937,11 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
}
-int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainChrDefPtr chr,
- bool async)
+static int
+qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainChrDefPtr chr,
+ bool async)
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -5992,7 +5995,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
}
-int
+static int
qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainRNGDefPtr rng,
@@ -6038,7 +6041,7 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
}
-int
+static int
qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainMemoryDefPtr memdef,
@@ -6086,7 +6089,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
}
-int
+static int
qemuDomainDetachInputDevice(virDomainObjPtr vm,
virDomainInputDefPtr def,
bool async)
@@ -6137,7 +6140,7 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm,
}
-int
+static int
qemuDomainDetachVsockDevice(virDomainObjPtr vm,
virDomainVsockDefPtr dev,
bool async)
@@ -6173,7 +6176,7 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm,
}
-int
+static int
qemuDomainDetachLease(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainLeaseDefPtr lease)
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 10593cf83c..9980c2a756 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -79,10 +79,6 @@ int qemuDomainFindGraphicsIndex(virDomainDefPtr def,
int qemuDomainAttachMemory(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainMemoryDefPtr mem);
-int qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr memdef,
- bool async);
int qemuDomainChangeGraphics(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainGraphicsDefPtr dev);
@@ -99,35 +95,6 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainNetDefPtr dev,
int linkstate);
-int qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async);
-int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async);
-int qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async);
-int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async);
-int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainShmemDefPtr dev,
- bool async);
-int qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainWatchdogDefPtr watchdog,
- bool async);
-
-int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainRedirdevDefPtr dev,
- bool async);
int qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -140,23 +107,12 @@ int qemuDomainAttachVsockDevice(virQEMUDriverPtr driver,
int qemuDomainAttachLease(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainLeaseDefPtr lease);
-int qemuDomainDetachLease(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainLeaseDefPtr lease);
int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainChrDefPtr chr);
-int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainChrDefPtr chr,
- bool async);
int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainRNGDefPtr rng);
-int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainRNGDefPtr rng,
- bool async);
int qemuDomainDetachDeviceLive(virDomainObjPtr vm,
virDomainDeviceDefPtr dev,
@@ -196,11 +152,4 @@ int qemuDomainSetVcpuInternal(virQEMUDriverPtr driver,
virBitmapPtr vcpus,
bool state);
-int qemuDomainDetachInputDevice(virDomainObjPtr vm,
- virDomainInputDefPtr def,
- bool async);
-
-int qemuDomainDetachVsockDevice(virDomainObjPtr vm,
- virDomainVsockDefPtr dev,
- bool async);
#endif /* LIBVIRT_QEMU_HOTPLUG_H */
--
2.20.1
8:48 a.m.
New subject: [libvirt] [PATCH v2 05/14] qemu_hotplug: make Detach functions called only from qemu_hotplug.c static
On Mon, Mar 25, 2019 at 13:24:27 -0400, Laine Stump wrote:
These are no longer called from qemu_driver.c, since the function
that
called them (qemuDomainDetachDeviceLive()) has been moved to
qemu_hotplug.c, and they are no longer called from testqemuhotplug.c
because it now just called qemuDomainDetachDeviceLive() instead of all
the subordinate functions.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH IN V2
src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++------------------
src/qemu/qemu_hotplug.h | 51 -----------------------------------------
2 files changed, 25 insertions(+), 73 deletions(-)
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 06/14] qemu_hotplug: rename dev to match in qemuDomainDetachDeviceLive
I'm about to add a second virDomainDeviceDef to this function that
will point to the actual device in the domain object. "dev" is
just a partially filled-in example of what to look for. Naming it
match will make the code easier to follow.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1: Eliminate movement of Chr and Lease Detach functions,
and addition of extra comments in switch. This has been moved to new
patch 07/14
src/qemu/qemu_hotplug.c | 32 ++++++++++++++++----------------
src/qemu/qemu_hotplug.h | 2 +-
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 50bfd5f4e6..18bf2da264 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6202,52 +6202,52 @@ qemuDomainDetachLease(virQEMUDriverPtr driver,
int
qemuDomainDetachDeviceLive(virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
+ virDomainDeviceDefPtr match,
virQEMUDriverPtr driver,
bool async)
{
int ret = -1;
- switch ((virDomainDeviceType)dev->type) {
+ switch ((virDomainDeviceType)match->type) {
case VIR_DOMAIN_DEVICE_DISK:
- ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async);
+ ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async);
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- ret = qemuDomainDetachControllerDevice(driver, vm, dev, async);
+ ret = qemuDomainDetachControllerDevice(driver, vm, match, async);
break;
case VIR_DOMAIN_DEVICE_LEASE:
- ret = qemuDomainDetachLease(driver, vm, dev->data.lease);
+ ret = qemuDomainDetachLease(driver, vm, match->data.lease);
break;
case VIR_DOMAIN_DEVICE_NET:
- ret = qemuDomainDetachNetDevice(driver, vm, dev, async);
+ ret = qemuDomainDetachNetDevice(driver, vm, match, async);
break;
case VIR_DOMAIN_DEVICE_HOSTDEV:
- ret = qemuDomainDetachHostDevice(driver, vm, dev, async);
+ ret = qemuDomainDetachHostDevice(driver, vm, match, async);
break;
case VIR_DOMAIN_DEVICE_CHR:
- ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr, async);
+ ret = qemuDomainDetachChrDevice(driver, vm, match->data.chr, async);
break;
case VIR_DOMAIN_DEVICE_RNG:
- ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng, async);
+ ret = qemuDomainDetachRNGDevice(driver, vm, match->data.rng, async);
break;
case VIR_DOMAIN_DEVICE_MEMORY:
- ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory, async);
+ ret = qemuDomainDetachMemoryDevice(driver, vm, match->data.memory, async);
break;
case VIR_DOMAIN_DEVICE_SHMEM:
- ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem, async);
+ ret = qemuDomainDetachShmemDevice(driver, vm, match->data.shmem, async);
break;
case VIR_DOMAIN_DEVICE_WATCHDOG:
- ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog, async);
+ ret = qemuDomainDetachWatchdog(driver, vm, match->data.watchdog, async);
break;
case VIR_DOMAIN_DEVICE_INPUT:
- ret = qemuDomainDetachInputDevice(vm, dev->data.input, async);
+ ret = qemuDomainDetachInputDevice(vm, match->data.input, async);
break;
case VIR_DOMAIN_DEVICE_REDIRDEV:
- ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev, async);
+ ret = qemuDomainDetachRedirdevDevice(driver, vm, match->data.redirdev,
async);
break;
case VIR_DOMAIN_DEVICE_VSOCK:
- ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async);
+ ret = qemuDomainDetachVsockDevice(vm, match->data.vsock, async);
break;
case VIR_DOMAIN_DEVICE_FS:
@@ -6265,7 +6265,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
case VIR_DOMAIN_DEVICE_LAST:
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("live detach of device '%s' is not
supported"),
- virDomainDeviceTypeToString(dev->type));
+ virDomainDeviceTypeToString(match->type));
break;
}
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 9980c2a756..78aa4e991b 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -115,7 +115,7 @@ int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
virDomainRNGDefPtr rng);
int qemuDomainDetachDeviceLive(virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
+ virDomainDeviceDefPtr match,
virQEMUDriverPtr driver,
bool async);
--
2.20.1
9:03 a.m.
New subject: [libvirt] [PATCH v2 06/14] qemu_hotplug: rename dev to match in qemuDomainDetachDeviceLive
On Mon, Mar 25, 2019 at 13:24:28 -0400, Laine Stump wrote:
I'm about to add a second virDomainDeviceDef to this function
that
will point to the actual device in the domain object. "dev" is
just a partially filled-in example of what to look for. Naming it
match will make the code easier to follow.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1: Eliminate movement of Chr and Lease Detach functions,
and addition of extra comments in switch. This has been moved to new
patch 07/14
src/qemu/qemu_hotplug.c | 32 ++++++++++++++++----------------
src/qemu/qemu_hotplug.h | 2 +-
2 files changed, 17 insertions(+), 17 deletions(-)
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 07/14] qemu_hotplug: separate Chr|Lease from other devices in DetachDevice switch
The Chr and Lease devices have detach code that is too different from
the other device types to handle with common functionality (which will
soon be added at the end of qemuDomainDetachDeviceLive(). In order to
make this difference obvious, move the cases for those two device
types to the top of the switch statement in
qemuDomainDetachDeviceLive(), have the cases return immediately so the
future common code at the end of the function will be skipped, and
also include some hopefully helpful comments to remind future
maintainers why these two device types are treated differently.
Any attempt to detach an unsupported device type should also skip the
future common code at the end of the function, so the case for
unsupported types is similarly changed from a simple break to a return
-1.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH IN V2.
src/qemu/qemu_hotplug.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 18bf2da264..c9b8868f9c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6209,24 +6209,36 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
int ret = -1;
switch ((virDomainDeviceType)match->type) {
+ /*
+ * lease and chr devices don't follow the standard pattern of
+ * the others, so they must have their own self-contained
+ * Detach functions.
+ */
+ case VIR_DOMAIN_DEVICE_LEASE:
+ return qemuDomainDetachLease(driver, vm, match->data.lease);
+
+ case VIR_DOMAIN_DEVICE_CHR:
+ return qemuDomainDetachChrDevice(driver, vm, match->data.chr, async);
+
+ /*
+ * All the other device types follow a very similar pattern -
+ * First we call type-specific functions to 1) locate the
+ * device we want to detach (based on the prototype device in
+ * match) and 2) do any device-type-specific validation to
+ * assure it is okay to detach the device.
+ */
case VIR_DOMAIN_DEVICE_DISK:
ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async);
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
ret = qemuDomainDetachControllerDevice(driver, vm, match, async);
break;
- case VIR_DOMAIN_DEVICE_LEASE:
- ret = qemuDomainDetachLease(driver, vm, match->data.lease);
- break;
case VIR_DOMAIN_DEVICE_NET:
ret = qemuDomainDetachNetDevice(driver, vm, match, async);
break;
case VIR_DOMAIN_DEVICE_HOSTDEV:
ret = qemuDomainDetachHostDevice(driver, vm, match, async);
break;
- case VIR_DOMAIN_DEVICE_CHR:
- ret = qemuDomainDetachChrDevice(driver, vm, match->data.chr, async);
- break;
case VIR_DOMAIN_DEVICE_RNG:
ret = qemuDomainDetachRNGDevice(driver, vm, match->data.rng, async);
break;
@@ -6266,7 +6278,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("live detach of device '%s' is not
supported"),
virDomainDeviceTypeToString(match->type));
- break;
+ return -1;
}
return ret;
--
2.20.1
9:17 a.m.
New subject: [libvirt] [PATCH v2 07/14] qemu_hotplug: separate Chr|Lease from other devices in DetachDevice switch
On Mon, Mar 25, 2019 at 13:24:29 -0400, Laine Stump wrote:
The Chr and Lease devices have detach code that is too different
from
the other device types to handle with common functionality (which will
soon be added at the end of qemuDomainDetachDeviceLive(). In order to
make this difference obvious, move the cases for those two device
types to the top of the switch statement in
qemuDomainDetachDeviceLive(), have the cases return immediately so the
future common code at the end of the function will be skipped, and
also include some hopefully helpful comments to remind future
maintainers why these two device types are treated differently.
Any attempt to detach an unsupported device type should also skip the
future common code at the end of the function, so the case for
unsupported types is similarly changed from a simple break to a return
-1.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 08/14] qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()
Most of these functions will soon contain only some setup for
detaching the device, not the detach code proper (since that code is
identical for these devices). Their device specific functions are all
being renamed to qemuDomainDetachPrep*(), where * is the
name of that device's data member in the virDomainDeviceDef
object.
Since there will be other code in qemuDomainDetachDeviceLive() after
the calls to qemuDomainDetachPrep*() that could still fail, we no
longer directly set "ret" with the return code from
qemuDomainDetachPrep*() functions, but simply return -1 on
failure, and wait until the end of qemuDomainDetachDeviceLive() to set
ret = 0.
Along with the rename, qemuDomainDetachPrep*() functions are also
given similar arglists, including an arg called "match" that points to
the proto-object of the device we want to delete, and another arg
"detach" that is used to return a pointer to the actual object that
will be (for now *has been*) detached. To make sure these new args
aren't confused with existing local pointers that sometimes had the
same name (detach), the local pointer to the device is now named after
the device type ("controller", "disk", etc). These point to the same
place as (*detach)->data.blah, it's just easier on the eyes to have,
e.g., "disk->dst" rather than "(*detach)->data.disk-dst".
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1:
g
* Renaming of Chr and Lease functions is now in a separate patch - 09/14.
* "reverting name change" made in previous patch" that was pointed out
by Peter is eliminated - I removed the name change from the earlier
patch prior to pushing, thus simplifying both patches.
* preserved NULL initialization of pointers that had it (no matter how unnecessary)
* I *haven't* removed ret from qemuDomainDetachDeviceLive() as
suggested in review, since it's just going to be added back in Patch
12/14 anyway.
src/qemu/qemu_hotplug.c | 317 +++++++++++++++++++++++-----------------
1 file changed, 182 insertions(+), 135 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c9b8868f9c..0c8e673381 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5390,21 +5390,22 @@ qemuFindDisk(virDomainDefPtr def, const char *dst)
}
static int
-qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async)
+qemuDomainDetachPrepDisk(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr match,
+ virDomainDiskDefPtr *detach,
+ bool async)
{
virDomainDiskDefPtr disk;
int idx;
int ret = -1;
- if ((idx = qemuFindDisk(vm->def, dev->data.disk->dst)) < 0) {
+ if ((idx = qemuFindDisk(vm->def, match->dst)) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED,
- _("disk %s not found"), dev->data.disk->dst);
+ _("disk %s not found"), match->dst);
return -1;
}
- disk = vm->def->disks[idx];
+ *detach = disk = vm->def->disks[idx];
switch ((virDomainDiskDevice) disk->device) {
case VIR_DOMAIN_DISK_DEVICE_DISK:
@@ -5541,57 +5542,55 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm,
}
static int
-qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async)
+qemuDomainDetachPrepController(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainControllerDefPtr match,
+ virDomainControllerDefPtr *detach,
+ bool async)
{
int idx, ret = -1;
- virDomainControllerDefPtr detach = NULL;
+ virDomainControllerDefPtr controller = NULL;
- if (dev->data.controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
+ if (match->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("'%s' controller cannot be hot unplugged."),
-
virDomainControllerTypeToString(dev->data.controller->type));
+ virDomainControllerTypeToString(match->type));
return -1;
}
- if ((idx = virDomainControllerFind(vm->def,
- dev->data.controller->type,
- dev->data.controller->idx)) < 0) {
+ if ((idx = virDomainControllerFind(vm->def, match->type, match->idx)) <
0) {
virReportError(VIR_ERR_DEVICE_MISSING,
_("controller %s:%d not found"),
-
virDomainControllerTypeToString(dev->data.controller->type),
- dev->data.controller->idx);
+ virDomainControllerTypeToString(match->type),
+ match->idx);
goto cleanup;
}
- detach = vm->def->controllers[idx];
+ *detach = controller = vm->def->controllers[idx];
- if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
+ if (qemuIsMultiFunctionDevice(vm->def, &controller->info)) {
virReportError(VIR_ERR_OPERATION_FAILED,
- _("cannot hot unplug multifunction PCI device: %s"),
- dev->data.disk->dst);
+ "%s", _("cannot hot unplug multifunction PCI
device"));
goto cleanup;
}
- if (qemuDomainControllerIsBusy(vm, detach)) {
+ if (qemuDomainControllerIsBusy(vm, controller)) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("device cannot be detached: device is busy"));
goto cleanup;
}
if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &detach->info);
+ qemuDomainMarkDeviceForRemoval(vm, &controller->info);
- if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0)
+ if (qemuDomainDeleteDevice(vm, controller->info.alias) < 0)
goto cleanup;
if (async) {
ret = 0;
} else {
if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveControllerDevice(driver, vm, detach);
+ ret = qemuDomainRemoveControllerDevice(driver, vm, controller);
}
cleanup:
@@ -5603,29 +5602,30 @@ qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
/* search for a hostdev matching dev and detach it */
static int
-qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async)
+qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainHostdevDefPtr match,
+ virDomainHostdevDefPtr *detach,
+ bool async)
{
- virDomainHostdevDefPtr hostdev = dev->data.hostdev;
- virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys;
+ virDomainHostdevSubsysPtr subsys = &match->source.subsys;
virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb;
virDomainHostdevSubsysPCIPtr pcisrc = &subsys->u.pci;
virDomainHostdevSubsysSCSIPtr scsisrc = &subsys->u.scsi;
virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev;
- virDomainHostdevDefPtr detach = NULL;
+ virDomainHostdevDefPtr hostdev = NULL;
int idx;
int ret = -1;
- if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
+ if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("hot unplug is not supported for hostdev mode
'%s'"),
- virDomainHostdevModeTypeToString(hostdev->mode));
+ virDomainHostdevModeTypeToString(match->mode));
return -1;
}
- idx = virDomainHostdevFind(vm->def, hostdev, &detach);
+ idx = virDomainHostdevFind(vm->def, match, &hostdev);
+ *detach = hostdev;
if (idx < 0) {
switch (subsys->type) {
@@ -5678,27 +5678,27 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
return -1;
}
- if (qemuIsMultiFunctionDevice(vm->def, detach->info)) {
+ if (qemuIsMultiFunctionDevice(vm->def, hostdev->info)) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("cannot hot unplug multifunction PCI device with guest
address: "
"%.4x:%.2x:%.2x.%.1x"),
- detach->info->addr.pci.domain,
detach->info->addr.pci.bus,
- detach->info->addr.pci.slot,
detach->info->addr.pci.function);
+ hostdev->info->addr.pci.domain,
hostdev->info->addr.pci.bus,
+ hostdev->info->addr.pci.slot,
hostdev->info->addr.pci.function);
return -1;
}
- if (!detach->info->alias) {
+ if (!hostdev->info->alias) {
virReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("device cannot be detached without a device
alias"));
return -1;
}
if (!async)
- qemuDomainMarkDeviceForRemoval(vm, detach->info);
+ qemuDomainMarkDeviceForRemoval(vm, hostdev->info);
- if (qemuDomainDeleteDevice(vm, detach->info->alias) < 0) {
+ if (qemuDomainDeleteDevice(vm, hostdev->info->alias) < 0) {
if (virDomainObjIsActive(vm))
- virDomainAuditHostdev(vm, detach, "detach", false);
+ virDomainAuditHostdev(vm, hostdev, "detach", false);
goto cleanup;
}
@@ -5706,7 +5706,7 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
ret = 0;
} else {
if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveHostDevice(driver, vm, detach);
+ ret = qemuDomainRemoveHostDevice(driver, vm, hostdev);
}
cleanup:
@@ -5719,24 +5719,25 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
static int
-qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainShmemDefPtr dev,
- bool async)
+qemuDomainDetachPrepShmem(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainShmemDefPtr match,
+ virDomainShmemDefPtr *detach,
+ bool async)
{
int ret = -1;
ssize_t idx = -1;
virDomainShmemDefPtr shmem = NULL;
- if ((idx = virDomainShmemDefFind(vm->def, dev)) < 0) {
+ if ((idx = virDomainShmemDefFind(vm->def, match)) < 0) {
virReportError(VIR_ERR_DEVICE_MISSING,
_("model '%s' shmem device not present "
"in domain configuration"),
- virDomainShmemModelTypeToString(dev->model));
+ virDomainShmemModelTypeToString(match->model));
return -1;
}
- shmem = vm->def->shmems[idx];
+ *detach = shmem = vm->def->shmems[idx];
switch ((virDomainShmemModel)shmem->model) {
case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN:
@@ -5773,13 +5774,17 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
static int
-qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainWatchdogDefPtr dev,
- bool async)
+qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainWatchdogDefPtr match,
+ virDomainWatchdogDefPtr *detach,
+ bool async)
{
int ret = -1;
- virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
+ virDomainWatchdogDefPtr watchdog;
+
+
+ *detach = watchdog = vm->def->watchdog;
if (!watchdog) {
virReportError(VIR_ERR_DEVICE_MISSING, "%s",
@@ -5790,9 +5795,9 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
/* While domains can have up to one watchdog, the one supplied by the user
* doesn't necessarily match the one domain has. Refuse to detach in such
* case. */
- if (!(watchdog->model == dev->model &&
- watchdog->action == dev->action &&
- virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info)))
{
+ if (!(watchdog->model == match->model &&
+ watchdog->action == match->action &&
+ virDomainDeviceInfoAddressIsEqual(&match->info,
&watchdog->info))) {
virReportError(VIR_ERR_DEVICE_MISSING,
_("model '%s' watchdog device not present "
"in domain configuration"),
@@ -5828,40 +5833,41 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
static int
-qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainRedirdevDefPtr dev,
- bool async)
+qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainRedirdevDefPtr match,
+ virDomainRedirdevDefPtr *detach,
+ bool async)
{
int ret = -1;
- virDomainRedirdevDefPtr tmpRedirdevDef;
+ virDomainRedirdevDefPtr redirdev;
ssize_t idx;
- if ((idx = virDomainRedirdevDefFind(vm->def, dev)) < 0) {
+ if ((idx = virDomainRedirdevDefFind(vm->def, match)) < 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("no matching redirdev was not found"));
return -1;
}
- tmpRedirdevDef = vm->def->redirdevs[idx];
+ *detach = redirdev = vm->def->redirdevs[idx];
- if (!tmpRedirdevDef->info.alias) {
+ if (!redirdev->info.alias) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("alias not set for redirdev device"));
return -1;
}
if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdevDef->info);
+ qemuDomainMarkDeviceForRemoval(vm, &redirdev->info);
- if (qemuDomainDeleteDevice(vm, tmpRedirdevDef->info.alias) < 0)
+ if (qemuDomainDeleteDevice(vm, redirdev->info.alias) < 0)
goto cleanup;
if (async) {
ret = 0;
} else {
if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdevDef);
+ ret = qemuDomainRemoveRedirdevDevice(driver, vm, redirdev);
}
cleanup:
@@ -5872,53 +5878,54 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
static int
-qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainDeviceDefPtr dev,
- bool async)
+qemuDomainDetachPrepNet(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainNetDefPtr match,
+ virDomainNetDefPtr *detach,
+ bool async)
{
int detachidx, ret = -1;
- virDomainNetDefPtr detach = NULL;
+ virDomainNetDefPtr net = NULL;
- if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
+ if ((detachidx = virDomainNetFindIdx(vm->def, match)) < 0)
goto cleanup;
- detach = vm->def->nets[detachidx];
+ *detach = net = vm->def->nets[detachidx];
- if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) {
+ if (qemuIsMultiFunctionDevice(vm->def, &net->info)) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("cannot hot unplug multifunction PCI device: %s"),
- detach->ifname);
+ net->ifname);
goto cleanup;
}
- if (!detach->info.alias) {
- if (qemuAssignDeviceNetAlias(vm->def, detach, -1) < 0)
+ if (!net->info.alias) {
+ if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0)
goto cleanup;
}
- if (virDomainNetGetActualBandwidth(detach) &&
- virNetDevSupportBandwidth(virDomainNetGetActualType(detach)) &&
- virNetDevBandwidthClear(detach->ifname) < 0)
+ if (virDomainNetGetActualBandwidth(net) &&
+ virNetDevSupportBandwidth(virDomainNetGetActualType(net)) &&
+ virNetDevBandwidthClear(net->ifname) < 0)
VIR_WARN("cannot clear bandwidth setting for device : %s",
- detach->ifname);
+ net->ifname);
/* deactivate the tap/macvtap device on the host, which could also
* affect the parent device (e.g. macvtap passthrough mode sets
* the parent device offline)
*/
- ignore_value(qemuInterfaceStopDevice(detach));
+ ignore_value(qemuInterfaceStopDevice(net));
if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &detach->info);
+ qemuDomainMarkDeviceForRemoval(vm, &net->info);
- if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) {
+ if (qemuDomainDeleteDevice(vm, net->info.alias) < 0) {
if (virDomainObjIsActive(vm)) {
/* the audit message has a different format for hostdev network devices */
- if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV)
- virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(detach),
"detach", false);
+ if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV)
+ virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(net),
"detach", false);
else
- virDomainAuditNet(vm, detach, NULL, "detach", false);
+ virDomainAuditNet(vm, net, NULL, "detach", false);
}
goto cleanup;
}
@@ -5927,7 +5934,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
ret = 0;
} else {
if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveNetDevice(driver, vm, detach);
+ ret = qemuDomainRemoveNetDevice(driver, vm, net);
}
cleanup:
@@ -5996,42 +6003,43 @@ qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
static int
-qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainRNGDefPtr rng,
- bool async)
+qemuDomainDetachPrepRNG(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainRNGDefPtr match,
+ virDomainRNGDefPtr *detach,
+ bool async)
{
ssize_t idx;
- virDomainRNGDefPtr tmpRNG;
+ virDomainRNGDefPtr rng;
int ret = -1;
- if ((idx = virDomainRNGFind(vm->def, rng)) < 0) {
+ if ((idx = virDomainRNGFind(vm->def, match)) < 0) {
virReportError(VIR_ERR_DEVICE_MISSING,
_("model '%s' RNG device not present "
"in domain configuration"),
- virDomainRNGBackendTypeToString(rng->model));
+ virDomainRNGBackendTypeToString(match->model));
return -1;
}
- tmpRNG = vm->def->rngs[idx];
+ *detach = rng = vm->def->rngs[idx];
- if (!tmpRNG->info.alias) {
+ if (!rng->info.alias) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("alias not set for RNG device"));
return -1;
}
if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info);
+ qemuDomainMarkDeviceForRemoval(vm, &rng->info);
- if (qemuDomainDeleteDevice(vm, tmpRNG->info.alias) < 0)
+ if (qemuDomainDeleteDevice(vm, rng->info.alias) < 0)
goto cleanup;
if (async) {
ret = 0;
} else {
if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG);
+ ret = qemuDomainRemoveRNGDevice(driver, vm, rng);
}
cleanup:
@@ -6042,26 +6050,27 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
static int
-qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainMemoryDefPtr memdef,
- bool async)
+qemuDomainDetachPrepMemory(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr match,
+ virDomainMemoryDefPtr *detach,
+ bool async)
{
virDomainMemoryDefPtr mem;
int idx;
int ret = -1;
- qemuDomainMemoryDeviceAlignSize(vm->def, memdef);
+ qemuDomainMemoryDeviceAlignSize(vm->def, match);
- if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) {
+ if ((idx = virDomainMemoryFindByDef(vm->def, match)) < 0) {
virReportError(VIR_ERR_DEVICE_MISSING,
_("model '%s' memory device not present "
"in the domain configuration"),
- virDomainMemoryModelTypeToString(memdef->model));
+ virDomainMemoryModelTypeToString(match->model));
return -1;
}
- mem = vm->def->mems[idx];
+ *detach = mem = vm->def->mems[idx];
if (!mem->info.alias) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -6090,20 +6099,21 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
static int
-qemuDomainDetachInputDevice(virDomainObjPtr vm,
- virDomainInputDefPtr def,
- bool async)
+qemuDomainDetachPrepInput(virDomainObjPtr vm,
+ virDomainInputDefPtr match,
+ virDomainInputDefPtr *detach,
+ bool async)
{
virDomainInputDefPtr input;
int ret = -1;
int idx;
- if ((idx = virDomainInputDefFind(vm->def, def)) < 0) {
+ if ((idx = virDomainInputDefFind(vm->def, match)) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("matching input device not found"));
return -1;
}
- input = vm->def->inputs[idx];
+ *detach = input = vm->def->inputs[idx];
switch ((virDomainInputBus) input->bus) {
case VIR_DOMAIN_INPUT_BUS_PS2:
@@ -6141,16 +6151,18 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm,
static int
-qemuDomainDetachVsockDevice(virDomainObjPtr vm,
- virDomainVsockDefPtr dev,
- bool async)
+qemuDomainDetachPrepVsock(virDomainObjPtr vm,
+ virDomainVsockDefPtr match,
+ virDomainVsockDefPtr *detach,
+ bool async)
{
- virDomainVsockDefPtr vsock = vm->def->vsock;
+ virDomainVsockDefPtr vsock;
int ret = -1;
+ *detach = vsock = vm->def->vsock;
if (!vsock ||
- !virDomainVsockDefEquals(dev, vsock)) {
+ !virDomainVsockDefEquals(match, vsock)) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("matching vsock device not found"));
return -1;
@@ -6206,6 +6218,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
virQEMUDriverPtr driver,
bool async)
{
+ virDomainDeviceDef detach = { .type = match->type };
int ret = -1;
switch ((virDomainDeviceType)match->type) {
@@ -6228,38 +6241,70 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
* assure it is okay to detach the device.
*/
case VIR_DOMAIN_DEVICE_DISK:
- ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async);
+ if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk,
+ &detach.data.disk, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- ret = qemuDomainDetachControllerDevice(driver, vm, match, async);
+ if (qemuDomainDetachPrepController(driver, vm, match->data.controller,
+ &detach.data.controller, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_NET:
- ret = qemuDomainDetachNetDevice(driver, vm, match, async);
+ if (qemuDomainDetachPrepNet(driver, vm, match->data.net,
+ &detach.data.net, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_HOSTDEV:
- ret = qemuDomainDetachHostDevice(driver, vm, match, async);
+ if (qemuDomainDetachPrepHostdev(driver, vm, match->data.hostdev,
+ &detach.data.hostdev, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_RNG:
- ret = qemuDomainDetachRNGDevice(driver, vm, match->data.rng, async);
+ if (qemuDomainDetachPrepRNG(driver, vm, match->data.rng,
+ &detach.data.rng, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_MEMORY:
- ret = qemuDomainDetachMemoryDevice(driver, vm, match->data.memory, async);
+ if (qemuDomainDetachPrepMemory(driver, vm, match->data.memory,
+ &detach.data.memory, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_SHMEM:
- ret = qemuDomainDetachShmemDevice(driver, vm, match->data.shmem, async);
+ if (qemuDomainDetachPrepShmem(driver, vm, match->data.shmem,
+ &detach.data.shmem, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_WATCHDOG:
- ret = qemuDomainDetachWatchdog(driver, vm, match->data.watchdog, async);
+ if (qemuDomainDetachPrepWatchdog(driver, vm, match->data.watchdog,
+ &detach.data.watchdog, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_INPUT:
- ret = qemuDomainDetachInputDevice(vm, match->data.input, async);
+ if (qemuDomainDetachPrepInput(vm, match->data.input,
+ &detach.data.input, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_REDIRDEV:
- ret = qemuDomainDetachRedirdevDevice(driver, vm, match->data.redirdev,
async);
+ if (qemuDomainDetachPrepRedirdev(driver, vm, match->data.redirdev,
+ &detach.data.redirdev, async) < 0) {
+ return -1;
+ }
break;
-
case VIR_DOMAIN_DEVICE_VSOCK:
- ret = qemuDomainDetachVsockDevice(vm, match->data.vsock, async);
+ if (qemuDomainDetachPrepVsock(vm, match->data.vsock,
+ &detach.data.vsock, async) < 0) {
+ return -1;
+ }
break;
case VIR_DOMAIN_DEVICE_FS:
@@ -6281,6 +6326,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
return -1;
}
+ ret = 0;
+
return ret;
}
--
2.20.1
1:40 p.m.
New subject: [libvirt] [PATCH v2 08/14] qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()
On Mon, Mar 25, 2019 at 13:24:30 -0400, Laine Stump wrote:
Most of these functions will soon contain only some setup for
detaching the device, not the detach code proper (since that code is
identical for these devices). Their device specific functions are all
being renamed to qemuDomainDetachPrep*(), where * is the
name of that device's data member in the virDomainDeviceDef
object.
Since there will be other code in qemuDomainDetachDeviceLive() after
the calls to qemuDomainDetachPrep*() that could still fail, we no
longer directly set "ret" with the return code from
qemuDomainDetachPrep*() functions, but simply return -1 on
failure, and wait until the end of qemuDomainDetachDeviceLive() to set
ret = 0.
Along with the rename, qemuDomainDetachPrep*() functions are also
given similar arglists, including an arg called "match" that points to
I must say that doing this along with the rename did not help
reviewability. The rename which includes whitespace change in the
argument list obscures actual argument changes.
I'd prefer if you did not do that in the future as I understand that
splitting this patch would be an even bigger nightmare.
the proto-object of the device we want to delete, and another arg
"detach" that is used to return a pointer to the actual object that
will be (for now *has been*) detached. To make sure these new args
aren't confused with existing local pointers that sometimes had the
same name (detach), the local pointer to the device is now named after
the device type ("controller", "disk", etc). These point to the same
place as (*detach)->data.blah, it's just easier on the eyes to have,
e.g., "disk->dst" rather than "(*detach)->data.disk-dst".
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1:
g
* Renaming of Chr and Lease functions is now in a separate patch - 09/14.
* "reverting name change" made in previous patch" that was pointed out
by Peter is eliminated - I removed the name change from the earlier
patch prior to pushing, thus simplifying both patches.
* preserved NULL initialization of pointers that had it (no matter how unnecessary)
* I *haven't* removed ret from qemuDomainDetachDeviceLive() as
suggested in review, since it's just going to be added back in Patch
12/14 anyway.
src/qemu/qemu_hotplug.c | 317 +++++++++++++++++++++++-----------------
1 file changed, 182 insertions(+), 135 deletions(-)
[...]
@@ -5773,13 +5774,17 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr
driver,
static int
-qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainWatchdogDefPtr dev,
- bool async)
+qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainWatchdogDefPtr match,
+ virDomainWatchdogDefPtr *detach,
+ bool async)
{
int ret = -1;
- virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
+ virDomainWatchdogDefPtr watchdog;
+
+
extra line
+ *detach = watchdog = vm->def->watchdog;
if (!watchdog) {
virReportError(VIR_ERR_DEVICE_MISSING, "%s",
[...]
@@ -6206,6 +6218,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
virQEMUDriverPtr driver,
bool async)
{
+ virDomainDeviceDef detach = { .type = match->type };
int ret = -1;
switch ((virDomainDeviceType)match->type) {
@@ -6228,38 +6241,70 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
* assure it is okay to detach the device.
*/
case VIR_DOMAIN_DEVICE_DISK:
- ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async);
+ if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk,
+ &detach.data.disk, async) < 0) {
+ return -1;
+ }
I'm not a fan of the curly braces on this single-line body but I was
told that the coding style mandates it. Thus I'll not require a change
here.
Also, all the functions should use the qemuHotplug prefix rather than
qemuDomain, but given that the file is inconsistent I'm not going to
insist.
ACK witht the extra line deleted. Please don't send further patches
which mix function renames and argument list change.
2:52 p.m.
New subject: [libvirt] [PATCH v2 08/14] qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()
On 3/26/19 8:40 AM, Peter Krempa wrote:
On Mon, Mar 25, 2019 at 13:24:30 -0400, Laine Stump wrote:
> Most of these functions will soon contain only some setup for
> detaching the device, not the detach code proper (since that code is
> identical for these devices). Their device specific functions are all
> being renamed to qemuDomainDetachPrep*(), where * is the
> name of that device's data member in the virDomainDeviceDef
> object.
>
> Since there will be other code in qemuDomainDetachDeviceLive() after
> the calls to qemuDomainDetachPrep*() that could still fail, we no
> longer directly set "ret" with the return code from
> qemuDomainDetachPrep*() functions, but simply return -1 on
> failure, and wait until the end of qemuDomainDetachDeviceLive() to set
> ret = 0.
>
> Along with the rename, qemuDomainDetachPrep*() functions are also
> given similar arglists, including an arg called "match" that points to
I must say that doing this along with the rename did not help
reviewability. The rename which includes whitespace change in the
argument list obscures actual argument changes.
I'd prefer if you did not do that in the future as I understand that
splitting this patch would be an even bigger nightmare.
Yeah, valid point. I just sometimes grow weary of changing a line in one
patch just to change the same line again another. But I guess I do that
enough anyway, so one more time shouldn't bother me :-P
I'll be sure to inflate my patch count as much as reasonably possible
next time!
> the proto-object of the device we want to delete, and another arg
> "detach" that is used to return a pointer to the actual object that
> will be (for now *has been*) detached. To make sure these new args
> aren't confused with existing local pointers that sometimes had the
> same name (detach), the local pointer to the device is now named after
> the device type ("controller", "disk", etc). These point to the
same
> place as (*detach)->data.blah, it's just easier on the eyes to have,
> e.g., "disk->dst" rather than "(*detach)->data.disk-dst".
>
> Signed-off-by: Laine Stump <laine(a)laine.org>
> ---
>
> Change from V1:
> g
> * Renaming of Chr and Lease functions is now in a separate patch - 09/14.
>
> * "reverting name change" made in previous patch" that was pointed
out
> by Peter is eliminated - I removed the name change from the earlier
> patch prior to pushing, thus simplifying both patches.
>
> * preserved NULL initialization of pointers that had it (no matter how unnecessary)
>
> * I *haven't* removed ret from qemuDomainDetachDeviceLive() as
> suggested in review, since it's just going to be added back in Patch
> 12/14 anyway.
>
> src/qemu/qemu_hotplug.c | 317 +++++++++++++++++++++++-----------------
> 1 file changed, 182 insertions(+), 135 deletions(-)
[...]
> @@ -5773,13 +5774,17 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>
>
> static int
> -qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
> - virDomainObjPtr vm,
> - virDomainWatchdogDefPtr dev,
> - bool async)
> +qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainWatchdogDefPtr match,
> + virDomainWatchdogDefPtr *detach,
> + bool async)
> {
> int ret = -1;
> - virDomainWatchdogDefPtr watchdog = vm->def->watchdog;
> + virDomainWatchdogDefPtr watchdog;
> +
> +
extra line
> + *detach = watchdog = vm->def->watchdog;
>
> if (!watchdog) {
> virReportError(VIR_ERR_DEVICE_MISSING, "%s",
[...]
> @@ -6206,6 +6218,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
> virQEMUDriverPtr driver,
> bool async)
> {
> + virDomainDeviceDef detach = { .type = match->type };
> int ret = -1;
>
> switch ((virDomainDeviceType)match->type) {
> @@ -6228,38 +6241,70 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
> * assure it is okay to detach the device.
> */
> case VIR_DOMAIN_DEVICE_DISK:
> - ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async);
> + if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk,
> + &detach.data.disk, async) < 0) {
> + return -1;
> + }
I'm not a fan of the curly braces on this single-line body but I was
told that the coding style mandates it. Thus I'll not require a change
here.
I'm not a fan of *not* putting curly braces here :-)
Also, all the functions should use the qemuHotplug prefix rather than
qemuDomain, but given that the file is inconsistent I'm not going to
insist.
Good point. I'm guessing it's because many others of these functions
also started out in qemu_driver.c (and at a time when were less
consistent with function naming) and were moved to this file with no
name change.
A followup patch to make the names all consistent might be nice...
ACK witht the extra line deleted. Please don't send further patches
which mix function renames and argument list change.
Yes sir!
6:24 p.m.
New subject: [libvirt] [PATCH v2 09/14] qemu_hotplug: rename Chr and Lease Detach functions
qemuDomainDetachDeviceChr and qemuDomainDetachDeviceLease are more
consistent with each other.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH in V2 (previously was part of 08/14)
src/qemu/qemu_hotplug.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0c8e673381..de30b08dd1 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5945,7 +5945,7 @@ qemuDomainDetachPrepNet(virQEMUDriverPtr driver,
static int
-qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
+qemuDomainDetachDeviceChr(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainChrDefPtr chr,
bool async)
@@ -6189,9 +6189,9 @@ qemuDomainDetachPrepVsock(virDomainObjPtr vm,
static int
-qemuDomainDetachLease(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virDomainLeaseDefPtr lease)
+qemuDomainDetachDeviceLease(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainLeaseDefPtr lease)
{
virDomainLeaseDefPtr det_lease;
int idx;
@@ -6228,10 +6228,10 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
* Detach functions.
*/
case VIR_DOMAIN_DEVICE_LEASE:
- return qemuDomainDetachLease(driver, vm, match->data.lease);
+ return qemuDomainDetachDeviceLease(driver, vm, match->data.lease);
case VIR_DOMAIN_DEVICE_CHR:
- return qemuDomainDetachChrDevice(driver, vm, match->data.chr, async);
+ return qemuDomainDetachDeviceChr(driver, vm, match->data.chr, async);
/*
* All the other device types follow a very similar pattern -
--
2.20.1
1:41 p.m.
New subject: [libvirt] [PATCH v2 09/14] qemu_hotplug: rename Chr and Lease Detach functions
On Mon, Mar 25, 2019 at 13:24:31 -0400, Laine Stump wrote:
qemuDomainDetachDeviceChr and qemuDomainDetachDeviceLease are more
consistent with each other.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH in V2 (previously was part of 08/14)
src/qemu/qemu_hotplug.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
This function can be called with a virDomainDevicePtr and whether or
not the removal was successful, and it will call the appropriate
virDomainAudit*() function with the appropriate args for whatever type
of device it's given (or do nothing, if that's appropriate). This
permits generalizing some code that currently has a separate copy for
each type of device.
NB: Although the function initially will be called only with
success=false, that has been made an argument so that in the future
(when the qemuDomainRemove*Device() functions have had their common
functionality consolidated into qemuDomainRemoveDevice()), this new
common code can call qemuDomainRemoveAuditDevice() for all types.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
change from V1:
* only audit device types that were previously audited on *failure* to
remove (this preserves previous behavior). Auditing of other device
types is now added in new patch 11/14.
* use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error
due to the new function not yet being used.
src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index de30b08dd1..92d4e7d0f9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
}
+static void ATTRIBUTE_UNUSED
+qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
+ virDomainDeviceDefPtr detach,
+ bool success)
+{
+ switch ((virDomainDeviceType)detach->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ virDomainAuditDisk(vm, detach->data.disk->src, NULL, "detach",
success);
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ virDomainAuditNet(vm, detach->data.net, NULL, "detach", success);
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success);
+ break;
+
+ case VIR_DOMAIN_DEVICE_INPUT:
+ case VIR_DOMAIN_DEVICE_CHR:
+ case VIR_DOMAIN_DEVICE_RNG:
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ case VIR_DOMAIN_DEVICE_SHMEM:
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ /*
+ * These devices are supposed to be audited, but current code
+ * doesn't audit on failure to remove the device.
+ */
+ break;
+
+
+ case VIR_DOMAIN_DEVICE_LEASE:
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ case VIR_DOMAIN_DEVICE_VSOCK:
+ /* These devices don't have associated audit logs */
+ break;
+
+ case VIR_DOMAIN_DEVICE_FS:
+ case VIR_DOMAIN_DEVICE_SOUND:
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ case VIR_DOMAIN_DEVICE_GRAPHICS:
+ case VIR_DOMAIN_DEVICE_HUB:
+ case VIR_DOMAIN_DEVICE_SMARTCARD:
+ case VIR_DOMAIN_DEVICE_MEMBALLOON:
+ case VIR_DOMAIN_DEVICE_NVRAM:
+ case VIR_DOMAIN_DEVICE_NONE:
+ case VIR_DOMAIN_DEVICE_TPM:
+ case VIR_DOMAIN_DEVICE_PANIC:
+ case VIR_DOMAIN_DEVICE_IOMMU:
+ case VIR_DOMAIN_DEVICE_LAST:
+ /* these will never happen, these devices can't be unplugged */
+ break;
+ }
+}
+
+
int
qemuDomainRemoveDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
--
2.20.1
1:50 p.m.
New subject: [libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
On Mon, Mar 25, 2019 at 13:24:32 -0400, Laine Stump wrote:
This function can be called with a virDomainDevicePtr and whether or
not the removal was successful, and it will call the appropriate
virDomainAudit*() function with the appropriate args for whatever type
of device it's given (or do nothing, if that's appropriate). This
permits generalizing some code that currently has a separate copy for
each type of device.
NB: Although the function initially will be called only with
success=false, that has been made an argument so that in the future
(when the qemuDomainRemove*Device() functions have had their common
functionality consolidated into qemuDomainRemoveDevice()), this new
common code can call qemuDomainRemoveAuditDevice() for all types.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
change from V1:
* only audit device types that were previously audited on *failure* to
remove (this preserves previous behavior). Auditing of other device
types is now added in new patch 11/14.
* use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error
due to the new function not yet being used.
src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index de30b08dd1..92d4e7d0f9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
}
+static void ATTRIBUTE_UNUSED
+qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
+ virDomainDeviceDefPtr detach,
+ bool success)
+{
+ switch ((virDomainDeviceType)detach->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ virDomainAuditDisk(vm, detach->data.disk->src, NULL, "detach",
success);
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ virDomainAuditNet(vm, detach->data.net, NULL, "detach", success);
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ virDomainAuditHostdev(vm, detach->data.hostdev, "detach",
success);
+ break;
+
+ case VIR_DOMAIN_DEVICE_INPUT:
+ case VIR_DOMAIN_DEVICE_CHR:
+ case VIR_DOMAIN_DEVICE_RNG:
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ case VIR_DOMAIN_DEVICE_SHMEM:
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ /*
+ * These devices are supposed to be audited, but current code
+ * doesn't audit on failure to remove the device.
+ */
+ break;
+
+
+ case VIR_DOMAIN_DEVICE_LEASE:
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ case VIR_DOMAIN_DEVICE_VSOCK:
+ /* These devices don't have associated audit logs */
+ break;
+
+ case VIR_DOMAIN_DEVICE_FS:
+ case VIR_DOMAIN_DEVICE_SOUND:
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ case VIR_DOMAIN_DEVICE_GRAPHICS:
+ case VIR_DOMAIN_DEVICE_HUB:
+ case VIR_DOMAIN_DEVICE_SMARTCARD:
+ case VIR_DOMAIN_DEVICE_MEMBALLOON:
+ case VIR_DOMAIN_DEVICE_NVRAM:
+ case VIR_DOMAIN_DEVICE_NONE:
+ case VIR_DOMAIN_DEVICE_TPM:
+ case VIR_DOMAIN_DEVICE_PANIC:
+ case VIR_DOMAIN_DEVICE_IOMMU:
+ case VIR_DOMAIN_DEVICE_LAST:
+ /* these will never happen, these devices can't be unplugged */
This is actually misleading. The guest can ask for removal of a PCI
device. It's a bug in libvirt that the backend removal functions along
with deleting of the definition is not implemented.
I'd just state that they currently don't have detach handlers
implemented.
ACK regardless as this requires more fixing out of the scope.
5:58 p.m.
New subject: [libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
On Mon, Mar 25, 2019 at 01:24:32PM -0400, Laine Stump wrote:
This function can be called with a virDomainDevicePtr and whether or
not the removal was successful, and it will call the appropriate
virDomainAudit*() function with the appropriate args for whatever type
of device it's given (or do nothing, if that's appropriate). This
permits generalizing some code that currently has a separate copy for
each type of device.
NB: Although the function initially will be called only with
success=false, that has been made an argument so that in the future
(when the qemuDomainRemove*Device() functions have had their common
functionality consolidated into qemuDomainRemoveDevice()), this new
common code can call qemuDomainRemoveAuditDevice() for all types.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
change from V1:
* only audit device types that were previously audited on *failure* to
remove (this preserves previous behavior). Auditing of other device
types is now added in new patch 11/14.
* use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error
due to the new function not yet being used.
src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index de30b08dd1..92d4e7d0f9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
}
+static void ATTRIBUTE_UNUSED
+qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
+ virDomainDeviceDefPtr detach,
+ bool success)
+{
+ switch ((virDomainDeviceType)detach->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ virDomainAuditDisk(vm, detach->data.disk->src, NULL, "detach",
success);
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ virDomainAuditNet(vm, detach->data.net, NULL, "detach", success);
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success);
+ break;
+
+ case VIR_DOMAIN_DEVICE_INPUT:
+ case VIR_DOMAIN_DEVICE_CHR:
+ case VIR_DOMAIN_DEVICE_RNG:
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ case VIR_DOMAIN_DEVICE_SHMEM:
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ /*
+ * These devices are supposed to be audited, but current code
+ * doesn't audit on failure to remove the device.
+ */
+ break;
Indentation is off here.
Jano
6:40 p.m.
New subject: [libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
On 3/26/19 12:58 PM, Ján Tomko wrote:
On Mon, Mar 25, 2019 at 01:24:32PM -0400, Laine Stump wrote:
> This function can be called with a virDomainDevicePtr and whether or
> not the removal was successful, and it will call the appropriate
> virDomainAudit*() function with the appropriate args for whatever type
> of device it's given (or do nothing, if that's appropriate). This
> permits generalizing some code that currently has a separate copy for
> each type of device.
>
> NB: Although the function initially will be called only with
> success=false, that has been made an argument so that in the future
> (when the qemuDomainRemove*Device() functions have had their common
> functionality consolidated into qemuDomainRemoveDevice()), this new
> common code can call qemuDomainRemoveAuditDevice() for all types.
>
> Signed-off-by: Laine Stump <laine(a)laine.org>
> ---
>
> change from V1:
>
> * only audit device types that were previously audited on *failure* to
> remove (this preserves previous behavior). Auditing of other device
> types is now added in new patch 11/14.
>
> * use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error
> due to the new function not yet being used.
>
> src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index de30b08dd1..92d4e7d0f9 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5208,6 +5208,61 @@
> qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
> }
>
>
> +static void ATTRIBUTE_UNUSED
> +qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
> + virDomainDeviceDefPtr detach,
> + bool success)
> +{
> + switch ((virDomainDeviceType)detach->type) {
> + case VIR_DOMAIN_DEVICE_DISK:
> + virDomainAuditDisk(vm, detach->data.disk->src, NULL,
> "detach", success);
> + break;
> + case VIR_DOMAIN_DEVICE_NET:
> + virDomainAuditNet(vm, detach->data.net, NULL, "detach",
> success);
> + break;
> + case VIR_DOMAIN_DEVICE_HOSTDEV:
> + virDomainAuditHostdev(vm, detach->data.hostdev, "detach",
> success);
> + break;
> +
> + case VIR_DOMAIN_DEVICE_INPUT:
> + case VIR_DOMAIN_DEVICE_CHR:
> + case VIR_DOMAIN_DEVICE_RNG:
> + case VIR_DOMAIN_DEVICE_MEMORY:
> + case VIR_DOMAIN_DEVICE_SHMEM:
> + case VIR_DOMAIN_DEVICE_REDIRDEV:
> + /*
> + * These devices are supposed to be audited, but current code
> + * doesn't audit on failure to remove the device.
> + */
> + break;
Indentation is off here.
Whew! This bothered me for a minute, since I'd already pushed
everything, but when I looked at what was pushed, I realized that Peter
requested different wording for the comment, and in the process of doing
that, I had accidentally fixed the indentation.
Thanks for the attention to detail though!
6:24 p.m.
New subject: [libvirt] [PATCH v2 11/14] qemu_hotplug: audit *all* auditable device types in qemuDomainRemoveAuditDevice
Although all hotpluggable devices other than lease, controller,
watchdof, and vsock can be audited, and *are* audited when an unplug
is successful, only disk, net, and hostdev were actually being audited
on failure.
This patch corrects that omission.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH in V2 - previously a part of patch 10/14
src/qemu/qemu_hotplug.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 92d4e7d0f9..e9d6c8622b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5223,19 +5223,28 @@ qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
case VIR_DOMAIN_DEVICE_HOSTDEV:
virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success);
break;
-
case VIR_DOMAIN_DEVICE_INPUT:
+ virDomainAuditInput(vm, detach->data.input, "detach", success);
+ break;
case VIR_DOMAIN_DEVICE_CHR:
+ virDomainAuditChardev(vm, detach->data.chr, NULL, "detach",
success);
+ break;
case VIR_DOMAIN_DEVICE_RNG:
- case VIR_DOMAIN_DEVICE_MEMORY:
+ virDomainAuditRNG(vm, detach->data.rng, NULL, "detach", success);
+ break;
+ case VIR_DOMAIN_DEVICE_MEMORY: {
+ unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def);
+ unsigned long long newmem = oldmem - detach->data.memory->size;
+
+ virDomainAuditMemory(vm, oldmem, newmem, "update", success);
+ break;
+ }
case VIR_DOMAIN_DEVICE_SHMEM:
+ virDomainAuditShmem(vm, detach->data.shmem, "detach", success);
+ break;
case VIR_DOMAIN_DEVICE_REDIRDEV:
- /*
- * These devices are supposed to be audited, but current code
- * doesn't audit on failure to remove the device.
- */
- break;
-
+ virDomainAuditRedirdev(vm, detach->data.redirdev, "detach",
success);
+ break;
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_CONTROLLER:
--
2.20.1
1:52 p.m.
New subject: [libvirt] [PATCH v2 11/14] qemu_hotplug: audit *all* auditable device types in qemuDomainRemoveAuditDevice
On Mon, Mar 25, 2019 at 13:24:33 -0400, Laine Stump wrote:
Although all hotpluggable devices other than lease, controller,
watchdof, and vsock can be audited, and *are* audited when an unplug
is successful, only disk, net, and hostdev were actually being audited
on failure.
This patch corrects that omission.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH in V2 - previously a part of patch 10/14
src/qemu/qemu_hotplug.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 92d4e7d0f9..e9d6c8622b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5223,19 +5223,28 @@ qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
case VIR_DOMAIN_DEVICE_HOSTDEV:
virDomainAuditHostdev(vm, detach->data.hostdev, "detach",
success);
break;
-
case VIR_DOMAIN_DEVICE_INPUT:
+ virDomainAuditInput(vm, detach->data.input, "detach", success);
+ break;
case VIR_DOMAIN_DEVICE_CHR:
+ virDomainAuditChardev(vm, detach->data.chr, NULL, "detach",
success);
+ break;
case VIR_DOMAIN_DEVICE_RNG:
- case VIR_DOMAIN_DEVICE_MEMORY:
+ virDomainAuditRNG(vm, detach->data.rng, NULL, "detach", success);
+ break;
+ case VIR_DOMAIN_DEVICE_MEMORY: {
+ unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def);
+ unsigned long long newmem = oldmem - detach->data.memory->size;
+
+ virDomainAuditMemory(vm, oldmem, newmem, "update", success);
This probably should also say "detach" as the rest does.
ACK
3:12 p.m.
New subject: [libvirt] [PATCH v2 11/14] qemu_hotplug: audit *all* auditable device types in qemuDomainRemoveAuditDevice
On 3/26/19 8:52 AM, Peter Krempa wrote:
On Mon, Mar 25, 2019 at 13:24:33 -0400, Laine Stump wrote:
> Although all hotpluggable devices other than lease, controller,
> watchdof, and vsock can be audited, and *are* audited when an unplug
> is successful, only disk, net, and hostdev were actually being audited
> on failure.
>
> This patch corrects that omission.
>
> Signed-off-by: Laine Stump <laine(a)laine.org>
> ---
>
> NEW PATCH in V2 - previously a part of patch 10/14
>
> src/qemu/qemu_hotplug.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 92d4e7d0f9..e9d6c8622b 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5223,19 +5223,28 @@ qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
> case VIR_DOMAIN_DEVICE_HOSTDEV:
> virDomainAuditHostdev(vm, detach->data.hostdev, "detach",
success);
> break;
> -
> case VIR_DOMAIN_DEVICE_INPUT:
> + virDomainAuditInput(vm, detach->data.input, "detach",
success);
> + break;
> case VIR_DOMAIN_DEVICE_CHR:
> + virDomainAuditChardev(vm, detach->data.chr, NULL, "detach",
success);
> + break;
> case VIR_DOMAIN_DEVICE_RNG:
> - case VIR_DOMAIN_DEVICE_MEMORY:
> + virDomainAuditRNG(vm, detach->data.rng, NULL, "detach",
success);
> + break;
> + case VIR_DOMAIN_DEVICE_MEMORY: {
> + unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def);
> + unsigned long long newmem = oldmem - detach->data.memory->size;
> +
> + virDomainAuditMemory(vm, oldmem, newmem, "update", success);
This probably should also say "detach" as the rest does.
...except that all the other memory audits always say "update" rather
than "detach" or "attach". Maybe the author decided to look at
"memory"
as a single entity that could get larger or smaller, rather than a
collection of several different distinct
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 12/14] qemu_hotplug: consolidate all common detach code in qemuDomainDetachDeviceLive
Now that all the qemuDomainDetachPrep*() functions look nearly
identical at the end, we can put one copy of that identical code in
qemuDomainDetachDeviceLive() at the point after the individual prep
functions have been called, and remove the duplicated code from all
the prep functions. The code to locate the target "detach" device
based on the "match" device remains, as do all device-type-specific
validations.
Unfortunately there are a few things going on at once in this patch,
which makes it a bit more difficult to follow than the others; it was
just impossible to do the changes in stages and still have a
buildable/testable tree at each step.
The other changes of note:
* The individual prep functions no longer need their driver or async
args, so those are removed, as are the local "ret" variables, since
in all cases the functions just directly return -1 or 0.
* Some of the prep functions were checking for a valid alias and/or
for attempts to detach a multifunction PCI device, but not all. In
fact, both checks are valid (or at least harmless) for *all* device
types, so they are removed from the prep functions, and done a
single time in the common function.
(any attempts to *create* an alias when there isn't one has been
removed, since that is doomed to failure anyway; the only way the
device wouldn't have an alias is if 1) the domain was created by
calling virsh qemu-attach to attach an existing qemu process to
libvirt, and 2) the qemu command that started said process used "old
style" arguments for creating devices that didn't have any device
ids. Even if we constructed a device id for one of these devices,
qemu wouldn't recognize it in the device_del command anyway, so we
may as well fail earlier with "device missing alias" rather than
failing later with "couldn't delete device net0".)
* Only one type of device has shutdown code that must not be called
until after *all* validation of the device is done (including
checking for multifunction PCI and valid alias, which is done in the
toplevel common code). For this reason, the Net function has been
split in two, with the 2nd half (qemuDomainDetachShutdownNet())
called from the common function, right before sending the delete
command to qemu.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1:
* netdev shutdown is *not* moved to RemoveNet function - yet! That is
a good idea, but it is being done in a separate patch (14/14) in
order to make it easy to revert if we find it causes a problem.
*
src/qemu/qemu_hotplug.c | 481 ++++++++++++----------------------------
1 file changed, 142 insertions(+), 339 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e9d6c8622b..c86d1fb4eb 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5208,7 +5208,7 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
}
-static void ATTRIBUTE_UNUSED
+static void
qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
virDomainDeviceDefPtr detach,
bool success)
@@ -5454,15 +5454,12 @@ qemuFindDisk(virDomainDefPtr def, const char *dst)
}
static int
-qemuDomainDetachPrepDisk(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepDisk(virDomainObjPtr vm,
virDomainDiskDefPtr match,
- virDomainDiskDefPtr *detach,
- bool async)
+ virDomainDiskDefPtr *detach)
{
virDomainDiskDefPtr disk;
int idx;
- int ret = -1;
if ((idx = qemuFindDisk(vm->def, match->dst)) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -5514,34 +5511,7 @@ qemuDomainDetachPrepDisk(virQEMUDriverPtr driver,
if (qemuDomainDiskBlockJobIsActive(disk))
return -1;
- if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO &&
- qemuIsMultiFunctionDevice(vm->def, &disk->info)) {
- virReportError(VIR_ERR_OPERATION_FAILED,
- _("cannot hot unplug multifunction PCI device: %s"),
- disk->dst);
- return -1;
- }
-
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &disk->info);
-
- if (qemuDomainDeleteDevice(vm, disk->info.alias) < 0) {
- if (virDomainObjIsActive(vm))
- virDomainAuditDisk(vm, disk->src, NULL, "detach", false);
- goto cleanup;
- }
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveDiskDevice(driver, vm, disk);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
@@ -5606,13 +5576,11 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm,
}
static int
-qemuDomainDetachPrepController(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepController(virDomainObjPtr vm,
virDomainControllerDefPtr match,
- virDomainControllerDefPtr *detach,
- bool async)
+ virDomainControllerDefPtr *detach)
{
- int idx, ret = -1;
+ int idx;
virDomainControllerDefPtr controller = NULL;
if (match->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
@@ -5627,50 +5595,26 @@ qemuDomainDetachPrepController(virQEMUDriverPtr driver,
_("controller %s:%d not found"),
virDomainControllerTypeToString(match->type),
match->idx);
- goto cleanup;
+ return -1;
}
*detach = controller = vm->def->controllers[idx];
- if (qemuIsMultiFunctionDevice(vm->def, &controller->info)) {
- virReportError(VIR_ERR_OPERATION_FAILED,
- "%s", _("cannot hot unplug multifunction PCI
device"));
- goto cleanup;
- }
-
if (qemuDomainControllerIsBusy(vm, controller)) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("device cannot be detached: device is busy"));
- goto cleanup;
- }
-
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &controller->info);
-
- if (qemuDomainDeleteDevice(vm, controller->info.alias) < 0)
- goto cleanup;
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveControllerDevice(driver, vm, controller);
+ return -1;
}
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
/* search for a hostdev matching dev and detach it */
static int
-qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepHostdev(virDomainObjPtr vm,
virDomainHostdevDefPtr match,
- virDomainHostdevDefPtr *detach,
- bool async)
+ virDomainHostdevDefPtr *detach)
{
virDomainHostdevSubsysPtr subsys = &match->source.subsys;
virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb;
@@ -5679,7 +5623,6 @@ qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver,
virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev;
virDomainHostdevDefPtr hostdev = NULL;
int idx;
- int ret = -1;
if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -5742,54 +5685,15 @@ qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver,
return -1;
}
- if (qemuIsMultiFunctionDevice(vm->def, hostdev->info)) {
- virReportError(VIR_ERR_OPERATION_FAILED,
- _("cannot hot unplug multifunction PCI device with guest
address: "
- "%.4x:%.2x:%.2x.%.1x"),
- hostdev->info->addr.pci.domain,
hostdev->info->addr.pci.bus,
- hostdev->info->addr.pci.slot,
hostdev->info->addr.pci.function);
- return -1;
- }
-
- if (!hostdev->info->alias) {
- virReportError(VIR_ERR_OPERATION_FAILED,
- "%s", _("device cannot be detached without a device
alias"));
- return -1;
- }
-
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, hostdev->info);
-
- if (qemuDomainDeleteDevice(vm, hostdev->info->alias) < 0) {
- if (virDomainObjIsActive(vm))
- virDomainAuditHostdev(vm, hostdev, "detach", false);
- goto cleanup;
- }
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveHostDevice(driver, vm, hostdev);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
-
- return ret;
-
+ return 0;
}
static int
-qemuDomainDetachPrepShmem(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepShmem(virDomainObjPtr vm,
virDomainShmemDefPtr match,
- virDomainShmemDefPtr *detach,
- bool async)
+ virDomainShmemDefPtr *detach)
{
- int ret = -1;
ssize_t idx = -1;
virDomainShmemDefPtr shmem = NULL;
@@ -5817,34 +5721,15 @@ qemuDomainDetachPrepShmem(virQEMUDriverPtr driver,
return -1;
}
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &shmem->info);
-
- if (qemuDomainDeleteDevice(vm, shmem->info.alias) < 0)
- goto cleanup;
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveShmemDevice(driver, vm, shmem);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
static int
-qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepWatchdog(virDomainObjPtr vm,
virDomainWatchdogDefPtr match,
- virDomainWatchdogDefPtr *detach,
- bool async)
+ virDomainWatchdogDefPtr *detach)
{
- int ret = -1;
virDomainWatchdogDefPtr watchdog;
@@ -5876,34 +5761,15 @@ qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver,
return -1;
}
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &watchdog->info);
-
- if (qemuDomainDeleteDevice(vm, watchdog->info.alias) < 0)
- goto cleanup;
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveWatchdog(driver, vm, watchdog);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
static int
-qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepRedirdev(virDomainObjPtr vm,
virDomainRedirdevDefPtr match,
- virDomainRedirdevDefPtr *detach,
- bool async)
+ virDomainRedirdevDefPtr *detach)
{
- int ret = -1;
virDomainRedirdevDefPtr redirdev;
ssize_t idx;
@@ -5915,59 +5781,41 @@ qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver,
*detach = redirdev = vm->def->redirdevs[idx];
- if (!redirdev->info.alias) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("alias not set for redirdev device"));
- return -1;
- }
-
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &redirdev->info);
-
- if (qemuDomainDeleteDevice(vm, redirdev->info.alias) < 0)
- goto cleanup;
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveRedirdevDevice(driver, vm, redirdev);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
static int
-qemuDomainDetachPrepNet(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepNet(virDomainObjPtr vm,
virDomainNetDefPtr match,
- virDomainNetDefPtr *detach,
- bool async)
+ virDomainNetDefPtr *detach)
{
- int detachidx, ret = -1;
+ int detachidx;
virDomainNetDefPtr net = NULL;
if ((detachidx = virDomainNetFindIdx(vm->def, match)) < 0)
- goto cleanup;
+ return -1;
*detach = net = vm->def->nets[detachidx];
- if (qemuIsMultiFunctionDevice(vm->def, &net->info)) {
- virReportError(VIR_ERR_OPERATION_FAILED,
- _("cannot hot unplug multifunction PCI device: %s"),
- net->ifname);
- goto cleanup;
- }
+ return 0;
+}
- if (!net->info.alias) {
- if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0)
- goto cleanup;
- }
+static void
+qemuDomainDetachShutdownNet(virDomainNetDefPtr net)
+{
+/*
+ * These operations are in a separate function from
+ * qemuDomainDetachPrepNet() because they can't be done until after
+ * we've validated that this device really can be removed - in
+ * particular we need to check for multifunction PCI devices and
+ * presence of a device alias, which isn't done until *after* the
+ * return from qemuDomainDetachPrepNet(). Since we've already passed
+ * the "point of no return", we ignore any errors, and trudge ahead
+ * with shutting down and detaching the device even if there is an
+ * error in one of these functions.
+ */
if (virDomainNetGetActualBandwidth(net) &&
virNetDevSupportBandwidth(virDomainNetGetActualType(net)) &&
virNetDevBandwidthClear(net->ifname) < 0)
@@ -5979,35 +5827,8 @@ qemuDomainDetachPrepNet(virQEMUDriverPtr driver,
* the parent device offline)
*/
ignore_value(qemuInterfaceStopDevice(net));
-
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &net->info);
-
- if (qemuDomainDeleteDevice(vm, net->info.alias) < 0) {
- if (virDomainObjIsActive(vm)) {
- /* the audit message has a different format for hostdev network devices */
- if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV)
- virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(net),
"detach", false);
- else
- virDomainAuditNet(vm, net, NULL, "detach", false);
- }
- goto cleanup;
- }
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveNetDevice(driver, vm, net);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
}
-
static int
qemuDomainDetachDeviceChr(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -6067,15 +5888,12 @@ qemuDomainDetachDeviceChr(virQEMUDriverPtr driver,
static int
-qemuDomainDetachPrepRNG(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepRNG(virDomainObjPtr vm,
virDomainRNGDefPtr match,
- virDomainRNGDefPtr *detach,
- bool async)
+ virDomainRNGDefPtr *detach)
{
ssize_t idx;
virDomainRNGDefPtr rng;
- int ret = -1;
if ((idx = virDomainRNGFind(vm->def, match)) < 0) {
virReportError(VIR_ERR_DEVICE_MISSING,
@@ -6087,42 +5905,17 @@ qemuDomainDetachPrepRNG(virQEMUDriverPtr driver,
*detach = rng = vm->def->rngs[idx];
- if (!rng->info.alias) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("alias not set for RNG device"));
- return -1;
- }
-
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &rng->info);
-
- if (qemuDomainDeleteDevice(vm, rng->info.alias) < 0)
- goto cleanup;
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveRNGDevice(driver, vm, rng);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
static int
-qemuDomainDetachPrepMemory(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainDetachPrepMemory(virDomainObjPtr vm,
virDomainMemoryDefPtr match,
- virDomainMemoryDefPtr *detach,
- bool async)
+ virDomainMemoryDefPtr *detach)
{
virDomainMemoryDefPtr mem;
int idx;
- int ret = -1;
qemuDomainMemoryDeviceAlignSize(vm->def, match);
@@ -6136,40 +5929,16 @@ qemuDomainDetachPrepMemory(virQEMUDriverPtr driver,
*detach = mem = vm->def->mems[idx];
- if (!mem->info.alias) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("alias for the memory device was not found"));
- return -1;
- }
-
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &mem->info);
-
- if (qemuDomainDeleteDevice(vm, mem->info.alias) < 0)
- goto cleanup;
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveMemoryDevice(driver, vm, mem);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
static int
qemuDomainDetachPrepInput(virDomainObjPtr vm,
virDomainInputDefPtr match,
- virDomainInputDefPtr *detach,
- bool async)
+ virDomainInputDefPtr *detach)
{
virDomainInputDefPtr input;
- int ret = -1;
int idx;
if ((idx = virDomainInputDefFind(vm->def, match)) < 0) {
@@ -6194,35 +5963,16 @@ qemuDomainDetachPrepInput(virDomainObjPtr vm,
break;
}
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &input->info);
-
- if (qemuDomainDeleteDevice(vm, input->info.alias) < 0)
- goto cleanup;
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveInputDevice(vm, input);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
static int
qemuDomainDetachPrepVsock(virDomainObjPtr vm,
virDomainVsockDefPtr match,
- virDomainVsockDefPtr *detach,
- bool async)
+ virDomainVsockDefPtr *detach)
{
virDomainVsockDefPtr vsock;
- int ret = -1;
-
*detach = vsock = vm->def->vsock;
if (!vsock ||
@@ -6232,23 +5982,7 @@ qemuDomainDetachPrepVsock(virDomainObjPtr vm,
return -1;
}
- if (!async)
- qemuDomainMarkDeviceForRemoval(vm, &vsock->info);
-
- if (qemuDomainDeleteDevice(vm, vsock->info.alias) < 0)
- goto cleanup;
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveVsockDevice(vm, vsock);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
+ return 0;
}
@@ -6283,6 +6017,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
bool async)
{
virDomainDeviceDef detach = { .type = match->type };
+ virDomainDeviceInfoPtr info = NULL;
int ret = -1;
switch ((virDomainDeviceType)match->type) {
@@ -6305,68 +6040,68 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
* assure it is okay to detach the device.
*/
case VIR_DOMAIN_DEVICE_DISK:
- if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk,
- &detach.data.disk, async) < 0) {
+ if (qemuDomainDetachPrepDisk(vm, match->data.disk,
+ &detach.data.disk) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- if (qemuDomainDetachPrepController(driver, vm, match->data.controller,
- &detach.data.controller, async) < 0) {
+ if (qemuDomainDetachPrepController(vm, match->data.controller,
+ &detach.data.controller) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_NET:
- if (qemuDomainDetachPrepNet(driver, vm, match->data.net,
- &detach.data.net, async) < 0) {
+ if (qemuDomainDetachPrepNet(vm, match->data.net,
+ &detach.data.net) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_HOSTDEV:
- if (qemuDomainDetachPrepHostdev(driver, vm, match->data.hostdev,
- &detach.data.hostdev, async) < 0) {
+ if (qemuDomainDetachPrepHostdev(vm, match->data.hostdev,
+ &detach.data.hostdev) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_RNG:
- if (qemuDomainDetachPrepRNG(driver, vm, match->data.rng,
- &detach.data.rng, async) < 0) {
+ if (qemuDomainDetachPrepRNG(vm, match->data.rng,
+ &detach.data.rng) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_MEMORY:
- if (qemuDomainDetachPrepMemory(driver, vm, match->data.memory,
- &detach.data.memory, async) < 0) {
+ if (qemuDomainDetachPrepMemory(vm, match->data.memory,
+ &detach.data.memory) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_SHMEM:
- if (qemuDomainDetachPrepShmem(driver, vm, match->data.shmem,
- &detach.data.shmem, async) < 0) {
+ if (qemuDomainDetachPrepShmem(vm, match->data.shmem,
+ &detach.data.shmem) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_WATCHDOG:
- if (qemuDomainDetachPrepWatchdog(driver, vm, match->data.watchdog,
- &detach.data.watchdog, async) < 0) {
+ if (qemuDomainDetachPrepWatchdog(vm, match->data.watchdog,
+ &detach.data.watchdog) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_INPUT:
if (qemuDomainDetachPrepInput(vm, match->data.input,
- &detach.data.input, async) < 0) {
+ &detach.data.input) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_REDIRDEV:
- if (qemuDomainDetachPrepRedirdev(driver, vm, match->data.redirdev,
- &detach.data.redirdev, async) < 0) {
+ if (qemuDomainDetachPrepRedirdev(vm, match->data.redirdev,
+ &detach.data.redirdev) < 0) {
return -1;
}
break;
case VIR_DOMAIN_DEVICE_VSOCK:
if (qemuDomainDetachPrepVsock(vm, match->data.vsock,
- &detach.data.vsock, async) < 0) {
+ &detach.data.vsock) < 0) {
return -1;
}
break;
@@ -6390,7 +6125,75 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
return -1;
}
- ret = 0;
+ /* "detach" now points to the actual device we want to detach */
+
+ if (!(info = virDomainDeviceGetInfo(&detach))) {
+ /*
+ * This should never happen, since all of the device types in
+ * the switch cases that end with a "break" instead of a
+ * return have a virDeviceInfo in them.
+ */
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("device of type '%s' has no device info"),
+ virDomainDeviceTypeToString(detach.type));
+ return -1;
+ }
+
+
+ /* Make generic validation checks common to all device types */
+
+ if (!info->alias) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot detach %s device with no alias"),
+ virDomainDeviceTypeToString(detach.type));
+ return -1;
+ }
+
+ if (qemuIsMultiFunctionDevice(vm->def, info)) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("cannot hot unplug %s device with multifunction PCI guest
address: "
+ "%.4x:%.2x:%.2x.%.1x"),
+ virDomainDeviceTypeToString(detach.type),
+ info->addr.pci.domain, info->addr.pci.bus,
+ info->addr.pci.slot, info->addr.pci.function);
+ return -1;
+ }
+
+
+ /*
+ * Do any device-specific shutdown that should be
+ * done after all validation checks, but before issuing the qemu
+ * command to delete the device. For now, the only type of device
+ * that has such shutdown needs is the net device.
+ */
+ if (detach.type == VIR_DOMAIN_DEVICE_NET)
+ qemuDomainDetachShutdownNet(detach.data.net);
+
+
+ /*
+ * Issue the qemu monitor command to delete the device (based on
+ * its alias), and optionally wait a short time in case the
+ * DEVICE_DELETED event arrives from qemu right away.
+ */
+ if (!async)
+ qemuDomainMarkDeviceForRemoval(vm, info);
+
+ if (qemuDomainDeleteDevice(vm, info->alias) < 0) {
+ if (virDomainObjIsActive(vm))
+ qemuDomainRemoveAuditDevice(vm, &detach, false);
+ goto cleanup;
+ }
+
+ if (async) {
+ ret = 0;
+ } else {
+ if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+ ret = qemuDomainRemoveDevice(driver, vm, &detach);
+ }
+
+ cleanup:
+ if (!async)
+ qemuDomainResetDeviceRemoval(vm);
return ret;
}
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH v2 12/14] qemu_hotplug: consolidate all common detach code in qemuDomainDetachDeviceLive
On Mon, Mar 25, 2019 at 13:24:34 -0400, Laine Stump wrote:
Now that all the qemuDomainDetachPrep*() functions look nearly
identical at the end, we can put one copy of that identical code in
qemuDomainDetachDeviceLive() at the point after the individual prep
functions have been called, and remove the duplicated code from all
the prep functions. The code to locate the target "detach" device
based on the "match" device remains, as do all device-type-specific
validations.
Unfortunately there are a few things going on at once in this patch,
which makes it a bit more difficult to follow than the others; it was
just impossible to do the changes in stages and still have a
buildable/testable tree at each step.
The other changes of note:
* The individual prep functions no longer need their driver or async
args, so those are removed, as are the local "ret" variables, since
in all cases the functions just directly return -1 or 0.
* Some of the prep functions were checking for a valid alias and/or
for attempts to detach a multifunction PCI device, but not all. In
fact, both checks are valid (or at least harmless) for *all* device
types, so they are removed from the prep functions, and done a
single time in the common function.
(any attempts to *create* an alias when there isn't one has been
removed, since that is doomed to failure anyway; the only way the
device wouldn't have an alias is if 1) the domain was created by
calling virsh qemu-attach to attach an existing qemu process to
libvirt, and 2) the qemu command that started said process used "old
style" arguments for creating devices that didn't have any device
ids. Even if we constructed a device id for one of these devices,
qemu wouldn't recognize it in the device_del command anyway, so we
may as well fail earlier with "device missing alias" rather than
failing later with "couldn't delete device net0".)
* Only one type of device has shutdown code that must not be called
until after *all* validation of the device is done (including
checking for multifunction PCI and valid alias, which is done in the
toplevel common code). For this reason, the Net function has been
split in two, with the 2nd half (qemuDomainDetachShutdownNet())
called from the common function, right before sending the delete
command to qemu.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1:
* netdev shutdown is *not* moved to RemoveNet function - yet! That is
a good idea, but it is being done in a separate patch (14/14) in
order to make it easy to revert if we find it causes a problem.
*
src/qemu/qemu_hotplug.c | 481 ++++++++++++----------------------------
1 file changed, 142 insertions(+), 339 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e9d6c8622b..c86d1fb4eb 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
[...]
- goto cleanup;
- }
-
- if (async) {
- ret = 0;
- } else {
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
- ret = qemuDomainRemoveNetDevice(driver, vm, net);
- }
-
- cleanup:
- if (!async)
- qemuDomainResetDeviceRemoval(vm);
- return ret;
}
-
We usually do two lines between functions so there's no need to break it
here.
static int
qemuDomainDetachDeviceChr(virQEMUDriverPtr driver,
virDomainObjPtr vm,
ACK
6:24 p.m.
New subject: [libvirt] [PATCH v2 13/14] qemu_hotplug: delay sending DEVICE_REMOVED event until after *all* teardown
The VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event is sent after qemu has
responded to a device_del command with a DEVICE_DELETED event. Before
queuing the event, *some* of the final teardown of the device's
trappings in libvirt is done, but not *all* of it. As a result, an
application may receive and process the DEVICE_REMOVED event before
libvirt has really finished with it.
Usually this doesn't cause a problem, but it can - in the case of the
bug report referenced below, vdsm is assigning a PCI device to a guest
with managed='no', using livirt's virNodeDeviceDetachFlags() and
virNodeDeviceReAttach() APIs. Immediately after receiving a
DEVICE_REMOVED event from libvirt signalling that the device had been
successfully unplugged, vdsm would cal virNodeDeviceReAttach() to
unbind the device from vfio-pci and rebind it to the host driverm but
because the event was received before libvirt had completely finished
processing the removal, that device was still on the "activeDevs"
list, and so virNodeDeviceReAttach() failed.
Experimentation with additional debug logs proved that libvirt would
always end up dispatching the DEVICE_REMOVED event before it had
removed the device from activeDevs (with a *much* greater difference
with managed='yes', since in that case the re-binding of the device
occurred after queuing the device).
Although the case of hostdev devices is the most extreme (since there
is so much involved in tearing down the device), *all* device types
suffer from the same problem - the DEVICE_REMOVED event is queued very
early in the qemuDomainRemove*Device() function for all of them,
resulting in a possibility of any application receiving the event
before libvirt has really finished with the device.
The solution is to save the device's alias (which is the only piece of
info from the device object that is needed for the event) at the
beginning of processing the device removal, and then queue the event
as a final act before returning. Since all of the
qemuDomainRemove*Device() functions (except
qemuDomainRemoveChrDevice()) are now called exclusively from
qemuDomainRemoveDevice() (which selects which of the subordinates to
call in a switch statement based on the type of device), the shortest
route to a solution is to doing the saving of alias, and later
queueing of the event, in the higher level qemuDomainRemoveDevice(),
and just completely remove the event-related code from all the
subordinate functions.
The single exception to this, as mentioned before, is
qemuDomainRemoveChrDevice(), which is still called from somewhere
other than qemuDomainRemoveDevice() (and has a separate arg used to
trigger different behavior when the chr device has targetType ==
GUESTFWD), so it must keep its original behavior intact, and must be
treated differently by qemuDomainRemoveDevice() (similar to the way
that qemuDomainDetachDeviceLive() treats chr and lease devices
differently from all the others).
Resolves: https://bugzilla.redhat.com/1658198
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1:
* reworded some comments based on review
* don't error out if the device has no DeviceInfo, instead just don't
sent the DEVICE_REMOVED event.
* Set DeviceInfoPtr to NULL after we've retrieved the alias from it.
src/qemu/qemu_hotplug.c | 145 +++++++++++++++++++---------------------
1 file changed, 69 insertions(+), 76 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c86d1fb4eb..78890f4bbe 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4501,7 +4501,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
{
qemuHotplugDiskSourceDataPtr diskbackend = NULL;
virDomainDeviceDef dev;
- virObjectEventPtr event;
size_t i;
qemuDomainObjPrivatePtr priv = vm->privateData;
int ret = -1;
@@ -4529,9 +4528,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
virDomainAuditDisk(vm, disk->src, NULL, "detach", true);
- event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
-
qemuDomainReleaseDeviceAddress(vm, &disk->info,
virDomainDiskGetSource(disk));
/* tear down disk security access */
@@ -4555,19 +4551,14 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
static int
-qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainRemoveControllerDevice(virDomainObjPtr vm,
virDomainControllerDefPtr controller)
{
- virObjectEventPtr event;
size_t i;
VIR_DEBUG("Removing controller %s from domain %p %s",
controller->info.alias, vm, vm->def->name);
- event = virDomainEventDeviceRemovedNewFromObj(vm, controller->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
-
for (i = 0; i < vm->def->ncontrollers; i++) {
if (vm->def->controllers[i] == controller) {
virDomainControllerRemove(vm->def, i);
@@ -4589,7 +4580,6 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def);
unsigned long long newmem = oldmem - mem->size;
- virObjectEventPtr event;
char *backendAlias = NULL;
int rc;
int idx;
@@ -4611,9 +4601,6 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
if (rc < 0)
return -1;
- event = virDomainEventDeviceRemovedNewFromObj(vm, mem->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
-
if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
virDomainMemoryRemove(vm->def, idx);
@@ -4693,7 +4680,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
virDomainNetDefPtr net = NULL;
- virObjectEventPtr event;
size_t i;
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -4737,9 +4723,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
goto cleanup;
}
- event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias);
- virObjectEventStateQueue(driver->domainEventState, event);
-
if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET) {
net = hostdev->parent.data.net;
@@ -4818,7 +4801,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
{
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
qemuDomainObjPrivatePtr priv = vm->privateData;
- virObjectEventPtr event;
char *hostnet_name = NULL;
char *charDevAlias = NULL;
size_t i;
@@ -4863,9 +4845,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
virDomainAuditNet(vm, net, NULL, "detach", true);
- event = virDomainEventDeviceRemovedNewFromObj(vm, net->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
-
for (i = 0; i < vm->def->nnets; i++) {
if (vm->def->nets[i] == net) {
virDomainNetRemove(vm->def, i);
@@ -4948,11 +4927,20 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
if (qemuDomainNamespaceTeardownChardev(vm, chr) < 0)
VIR_WARN("Unable to remove chr device from /dev");
+ qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL);
+ qemuDomainChrRemove(vm->def, chr);
+
+ /* NB: all other qemuDomainRemove*Device() functions omit the
+ * sending of the DEVICE_REMOVED event, because they are *only*
+ * called from qemuDomainRemoveDevice(), and that function sends
+ * the DEVICE_REMOVED event for them, this function is different -
+ * it is called from other places than just
+ * qemuDomainRemoveDevice(), so it must send the DEVICE_REMOVED
+ * event itself.
+ */
event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias);
virObjectEventStateQueue(driver->domainEventState, event);
- qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL);
- qemuDomainChrRemove(vm->def, chr);
virDomainChrDefFree(chr);
ret = 0;
@@ -4967,7 +4955,6 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainRNGDefPtr rng)
{
- virObjectEventPtr event;
char *charAlias = NULL;
char *objAlias = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -5016,9 +5003,6 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
if (qemuDomainNamespaceTeardownRNG(vm, rng) < 0)
VIR_WARN("Unable to remove RNG device from /dev");
- event = virDomainEventDeviceRemovedNewFromObj(vm, rng->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
-
if ((idx = virDomainRNGFind(vm->def, rng)) >= 0)
virDomainRNGRemove(vm->def, idx);
qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL);
@@ -5043,7 +5027,6 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
char *charAlias = NULL;
char *memAlias = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
- virObjectEventPtr event = NULL;
VIR_DEBUG("Removing shmem device %s from domain %p %s",
shmem->info.alias, vm, vm->def->name);
@@ -5071,9 +5054,6 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
if (rc < 0)
goto cleanup;
- event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
-
if ((idx = virDomainShmemDefFind(vm->def, shmem)) >= 0)
virDomainShmemDefRemove(vm->def, idx);
qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL);
@@ -5089,17 +5069,12 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
static int
-qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
+qemuDomainRemoveWatchdog(virDomainObjPtr vm,
virDomainWatchdogDefPtr watchdog)
{
- virObjectEventPtr event = NULL;
-
VIR_DEBUG("Removing watchdog %s from domain %p %s",
watchdog->info.alias, vm, vm->def->name);
- event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
virDomainWatchdogDefFree(vm->def->watchdog);
vm->def->watchdog = NULL;
@@ -5111,16 +5086,11 @@ static int
qemuDomainRemoveInputDevice(virDomainObjPtr vm,
virDomainInputDefPtr dev)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- virQEMUDriverPtr driver = priv->driver;
- virObjectEventPtr event = NULL;
size_t i;
VIR_DEBUG("Removing input device %s from domain %p %s",
dev->info.alias, vm, vm->def->name);
- event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
for (i = 0; i < vm->def->ninputs; i++) {
if (vm->def->inputs[i] == dev)
break;
@@ -5145,15 +5115,9 @@ static int
qemuDomainRemoveVsockDevice(virDomainObjPtr vm,
virDomainVsockDefPtr dev)
{
- qemuDomainObjPrivatePtr priv = vm->privateData;
- virQEMUDriverPtr driver = priv->driver;
- virObjectEventPtr event = NULL;
-
VIR_DEBUG("Removing vsock device %s from domain %p %s",
dev->info.alias, vm, vm->def->name);
- event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL);
virDomainVsockDefFree(vm->def->vsock);
vm->def->vsock = NULL;
@@ -5167,7 +5131,6 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
virDomainRedirdevDefPtr dev)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- virObjectEventPtr event;
char *charAlias = NULL;
ssize_t idx;
int ret = -1;
@@ -5192,9 +5155,6 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
virDomainAuditRedirdev(vm, dev, "detach", true);
- event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
- virObjectEventStateQueue(driver->domainEventState, event);
-
if ((idx = virDomainRedirdevDefFind(vm->def, dev)) >= 0)
virDomainRedirdevDefRemove(vm->def, idx);
qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL);
@@ -5277,50 +5237,79 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
{
- int ret = -1;
+ virDomainDeviceInfoPtr info;
+ virObjectEventPtr event;
+ VIR_AUTOFREE(char *)alias = NULL;
+
+ /*
+ * save the alias to use when sending a DEVICE_REMOVED event after
+ * all other teardown is complete
+ */
+ if ((info = virDomainDeviceGetInfo(dev))
+ && VIR_STRDUP(alias, info->alias) < 0) {
+ return -1;
+ }
+ info = NULL; /* to prevent accidental use later */
+
switch ((virDomainDeviceType)dev->type) {
+ case VIR_DOMAIN_DEVICE_CHR:
+ /* We must return directly after calling
+ * qemuDomainRemoveChrDevice because it is called directly
+ * from other places, so it must be completely self-contained
+ * and can't take advantage of any common code at the end of
+ * qemuDomainRemoveDevice().
+ */
+ return qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true);
+
+ /*
+ * all of the following qemuDomainRemove*Device() functions
+ * are (and must be) only called from this function, so any
+ * code that is common to them all can be pulled out and put
+ * into this function.
+ */
case VIR_DOMAIN_DEVICE_DISK:
- ret = qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk);
+ if (qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk) < 0)
+ return -1;
break;
case VIR_DOMAIN_DEVICE_CONTROLLER:
- ret = qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller);
+ if (qemuDomainRemoveControllerDevice(vm, dev->data.controller) < 0)
+ return -1;
break;
case VIR_DOMAIN_DEVICE_NET:
- ret = qemuDomainRemoveNetDevice(driver, vm, dev->data.net);
+ if (qemuDomainRemoveNetDevice(driver, vm, dev->data.net) < 0)
+ return -1;
break;
case VIR_DOMAIN_DEVICE_HOSTDEV:
- ret = qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev);
- break;
-
- case VIR_DOMAIN_DEVICE_CHR:
- ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true);
+ if (qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0)
+ return -1;
break;
case VIR_DOMAIN_DEVICE_RNG:
- ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng);
+ if (qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng) < 0)
+ return -1;
break;
-
case VIR_DOMAIN_DEVICE_MEMORY:
- ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory);
+ if (qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory) < 0)
+ return -1;
break;
-
case VIR_DOMAIN_DEVICE_SHMEM:
- ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem);
+ if (qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem) < 0)
+ return -1;
break;
-
case VIR_DOMAIN_DEVICE_INPUT:
- ret = qemuDomainRemoveInputDevice(vm, dev->data.input);
+ if (qemuDomainRemoveInputDevice(vm, dev->data.input) < 0)
+ return -1;
break;
-
case VIR_DOMAIN_DEVICE_REDIRDEV:
- ret = qemuDomainRemoveRedirdevDevice(driver, vm, dev->data.redirdev);
+ if (qemuDomainRemoveRedirdevDevice(driver, vm, dev->data.redirdev) < 0)
+ return -1;
break;
-
case VIR_DOMAIN_DEVICE_WATCHDOG:
- ret = qemuDomainRemoveWatchdog(driver, vm, dev->data.watchdog);
+ if (qemuDomainRemoveWatchdog(vm, dev->data.watchdog) < 0)
+ return -1;
break;
-
case VIR_DOMAIN_DEVICE_VSOCK:
- ret = qemuDomainRemoveVsockDevice(vm, dev->data.vsock);
+ if (qemuDomainRemoveVsockDevice(vm, dev->data.vsock) < 0)
+ return -1;
break;
case VIR_DOMAIN_DEVICE_NONE:
@@ -5342,7 +5331,11 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
virDomainDeviceTypeToString(dev->type));
break;
}
- return ret;
+
+ event = virDomainEventDeviceRemovedNewFromObj(vm, alias);
+ virObjectEventStateQueue(driver->domainEventState, event);
+
+ return 0;
}
--
2.20.1
1:15 p.m.
New subject: [libvirt] [PATCH v2 13/14] qemu_hotplug: delay sending DEVICE_REMOVED event until after *all* teardown
On Mon, Mar 25, 2019 at 13:24:35 -0400, Laine Stump wrote:
The VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event is sent after qemu has
responded to a device_del command with a DEVICE_DELETED event. Before
queuing the event, *some* of the final teardown of the device's
trappings in libvirt is done, but not *all* of it. As a result, an
application may receive and process the DEVICE_REMOVED event before
libvirt has really finished with it.
Usually this doesn't cause a problem, but it can - in the case of the
bug report referenced below, vdsm is assigning a PCI device to a guest
with managed='no', using livirt's virNodeDeviceDetachFlags() and
virNodeDeviceReAttach() APIs. Immediately after receiving a
DEVICE_REMOVED event from libvirt signalling that the device had been
successfully unplugged, vdsm would cal virNodeDeviceReAttach() to
unbind the device from vfio-pci and rebind it to the host driverm but
because the event was received before libvirt had completely finished
processing the removal, that device was still on the "activeDevs"
list, and so virNodeDeviceReAttach() failed.
Experimentation with additional debug logs proved that libvirt would
always end up dispatching the DEVICE_REMOVED event before it had
removed the device from activeDevs (with a *much* greater difference
with managed='yes', since in that case the re-binding of the device
occurred after queuing the device).
Although the case of hostdev devices is the most extreme (since there
is so much involved in tearing down the device), *all* device types
suffer from the same problem - the DEVICE_REMOVED event is queued very
early in the qemuDomainRemove*Device() function for all of them,
resulting in a possibility of any application receiving the event
before libvirt has really finished with the device.
The solution is to save the device's alias (which is the only piece of
info from the device object that is needed for the event) at the
beginning of processing the device removal, and then queue the event
as a final act before returning. Since all of the
qemuDomainRemove*Device() functions (except
qemuDomainRemoveChrDevice()) are now called exclusively from
qemuDomainRemoveDevice() (which selects which of the subordinates to
call in a switch statement based on the type of device), the shortest
route to a solution is to doing the saving of alias, and later
queueing of the event, in the higher level qemuDomainRemoveDevice(),
and just completely remove the event-related code from all the
subordinate functions.
The single exception to this, as mentioned before, is
qemuDomainRemoveChrDevice(), which is still called from somewhere
other than qemuDomainRemoveDevice() (and has a separate arg used to
trigger different behavior when the chr device has targetType ==
GUESTFWD), so it must keep its original behavior intact, and must be
treated differently by qemuDomainRemoveDevice() (similar to the way
that qemuDomainDetachDeviceLive() treats chr and lease devices
differently from all the others).
Resolves: https://bugzilla.redhat.com/1658198
Signed-off-by: Laine Stump <laine(a)laine.org>
---
Change from V1:
* reworded some comments based on review
* don't error out if the device has no DeviceInfo, instead just don't
sent the DEVICE_REMOVED event.
* Set DeviceInfoPtr to NULL after we've retrieved the alias from it.
src/qemu/qemu_hotplug.c | 145 +++++++++++++++++++---------------------
1 file changed, 69 insertions(+), 76 deletions(-)
[...]
@@ -4948,11 +4927,20 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr
driver,
if (qemuDomainNamespaceTeardownChardev(vm, chr) < 0)
VIR_WARN("Unable to remove chr device from /dev");
+ qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL);
+ qemuDomainChrRemove(vm->def, chr);
+
+ /* NB: all other qemuDomainRemove*Device() functions omit the
+ * sending of the DEVICE_REMOVED event, because they are *only*
+ * called from qemuDomainRemoveDevice(), and that function sends
+ * the DEVICE_REMOVED event for them, this function is different -
+ * it is called from other places than just
+ * qemuDomainRemoveDevice(), so it must send the DEVICE_REMOVED
+ * event itself.
I think this should only say that this function needs to report the
event itself. Also mention that it has to happen after all the backend
stuff is detached.
/* The caller does not emit the event. Note that the event should be
* reported only after all backend stuff is gone
*/
+ */
event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias);
virObjectEventStateQueue(driver->domainEventState, event);
- qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL);
- qemuDomainChrRemove(vm->def, chr);
virDomainChrDefFree(chr);
ret = 0;
[...]
@@ -5277,50 +5237,79 @@ qemuDomainRemoveDevice(virQEMUDriverPtr
driver,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev)
{
- int ret = -1;
+ virDomainDeviceInfoPtr info;
+ virObjectEventPtr event;
+ VIR_AUTOFREE(char *)alias = NULL;
Missing space before 'alias'
+
+ /*
+ * save the alias to use when sending a DEVICE_REMOVED event after
+ * all other teardown is complete
+ */
+ if ((info = virDomainDeviceGetInfo(dev))
+ && VIR_STRDUP(alias, info->alias) < 0) {
&& is usually at the end of the previous line.
+ return -1;
+ }
+ info = NULL; /* to prevent accidental use later */
// this is bridge [1]
+
switch ((virDomainDeviceType)dev->type) {
ACK with the above addressed
[1] https://imgur.com/gallery/dZe8k
6:02 p.m.
New subject: [libvirt] [PATCH v2 13/14] qemu_hotplug: delay sending DEVICE_REMOVED event until after *all* teardown
On Tue, Mar 26, 2019 at 01:15:33PM +0100, Peter Krempa wrote:
On Mon, Mar 25, 2019 at 13:24:35 -0400, Laine Stump wrote:
> + return -1;
> + }
> + info = NULL; /* to prevent accidental use later */
// this is bridge [1]
> +
> switch ((virDomainDeviceType)dev->type) {
ACK with the above addressed
[1] https://imgur.com/gallery/dZe8k
Now with more credit to the author:
https://www.abstrusegoose.com/432
Jano
6:24 p.m.
New subject: [libvirt] [PATCH v2 14/14] qemu_hotplug: don't shutdown net device until the guest has released it
For [some unknown reason, possibly/probably pure chance], Net devices
have been taken offline and their bandwidth tc rules cleared as the
very first operation when detaching the device. This is contrary to
every other type of device, where all hostside teardown is delayed
until we receive the DEVICE_DELETED event back from qemu, indicating
that the guest has finished with the device.
This patch delays these two operations until receipt of
DEVICE_DELETED, which removes an ugly wart from
qemuDomainDetachDeviceLive(), and also seems to be a more correct
sequence of events.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH IN V2.
src/qemu/qemu_hotplug.c | 49 +++++++++--------------------------------
1 file changed, 11 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 78890f4bbe..2e2e913b4e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4821,6 +4821,17 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
!(charDevAlias = qemuAliasChardevFromDevAlias(net->info.alias)))
goto cleanup;
+ if (virDomainNetGetActualBandwidth(net) &&
+ virNetDevSupportBandwidth(virDomainNetGetActualType(net)) &&
+ virNetDevBandwidthClear(net->ifname) < 0)
+ VIR_WARN("cannot clear bandwidth setting for device : %s",
+ net->ifname);
+
+ /* deactivate the tap/macvtap device on the host, which could also
+ * affect the parent device (e.g. macvtap passthrough mode sets
+ * the parent device offline)
+ */
+ ignore_value(qemuInterfaceStopDevice(net));
qemuDomainObjEnterMonitor(driver, vm);
if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) {
@@ -5795,33 +5806,6 @@ qemuDomainDetachPrepNet(virDomainObjPtr vm,
}
-static void
-qemuDomainDetachShutdownNet(virDomainNetDefPtr net)
-{
-/*
- * These operations are in a separate function from
- * qemuDomainDetachPrepNet() because they can't be done until after
- * we've validated that this device really can be removed - in
- * particular we need to check for multifunction PCI devices and
- * presence of a device alias, which isn't done until *after* the
- * return from qemuDomainDetachPrepNet(). Since we've already passed
- * the "point of no return", we ignore any errors, and trudge ahead
- * with shutting down and detaching the device even if there is an
- * error in one of these functions.
- */
- if (virDomainNetGetActualBandwidth(net) &&
- virNetDevSupportBandwidth(virDomainNetGetActualType(net)) &&
- virNetDevBandwidthClear(net->ifname) < 0)
- VIR_WARN("cannot clear bandwidth setting for device : %s",
- net->ifname);
-
- /* deactivate the tap/macvtap device on the host, which could also
- * affect the parent device (e.g. macvtap passthrough mode sets
- * the parent device offline)
- */
- ignore_value(qemuInterfaceStopDevice(net));
-}
-
static int
qemuDomainDetachDeviceChr(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -6152,17 +6136,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
return -1;
}
-
- /*
- * Do any device-specific shutdown that should be
- * done after all validation checks, but before issuing the qemu
- * command to delete the device. For now, the only type of device
- * that has such shutdown needs is the net device.
- */
- if (detach.type == VIR_DOMAIN_DEVICE_NET)
- qemuDomainDetachShutdownNet(detach.data.net);
-
-
/*
* Issue the qemu monitor command to delete the device (based on
* its alias), and optionally wait a short time in case the
--
2.20.1
2:11 p.m.
New subject: [libvirt] [PATCH v2 14/14] qemu_hotplug: don't shutdown net device until the guest has released it
On Mon, Mar 25, 2019 at 13:24:36 -0400, Laine Stump wrote:
For [some unknown reason, possibly/probably pure chance], Net
devices
have been taken offline and their bandwidth tc rules cleared as the
very first operation when detaching the device. This is contrary to
every other type of device, where all hostside teardown is delayed
until we receive the DEVICE_DELETED event back from qemu, indicating
that the guest has finished with the device.
This patch delays these two operations until receipt of
DEVICE_DELETED, which removes an ugly wart from
qemuDomainDetachDeviceLive(), and also seems to be a more correct
sequence of events.
Signed-off-by: Laine Stump <laine(a)laine.org>
---
NEW PATCH IN V2.
Looks good to me although I don't have much knowledge in this regions.
Michal, could you please have a look?
2004
days inactive
2005
days old
34 comments
3 participants
participants (3)
-
Ján Tomko -
Laine Stump -
Peter Krempa