[libvirt] [PATCH v2 00/12] Introduce virDomainDetachDeviceAlias
v2 of: https://www.redhat.com/archives/libvir-list/2018-May/msg01530.html diff to v1: - The semantic is changed a bit. Now it's "send request" only, and users have to listen for DEVICE_DELETED event. They would have to do that anyway. - Couple of more fixes related to asynchronous behaviour Michal Privoznik (12): Introduce virDomainDetachDeviceAlias API remote: Implement virDomainDetachDeviceAlias virsh: Expose virDomainDetachDeviceAlias qemuDomainRemoveChrDevice: Release device address qemuDomainDetachShmemDevice: Don't release shmem address twice qemuDomainDetachWatchdog: Don't release watchdog address twice qemu_hotplug: Use more gotos in qemuDomainDetach*Device qemuDomainDetachDeviceLiveAndConfig: Don't use driver->caps directly qemuDomainDetachDeviceLiveAndConfig: Avoid overwriting @ret qemu_hotplug: Allow asynchronous detach qemu: Implement virDomainDetachDeviceAlias news: Document new API introduction docs/news.xml | 8 ++ include/libvirt/libvirt-domain.h | 3 + src/driver-hypervisor.h | 6 + src/libvirt-domain.c | 53 ++++++++ src/libvirt_public.syms | 5 + src/qemu/qemu_driver.c | 162 +++++++++++++++++++---- src/qemu/qemu_hotplug.c | 278 +++++++++++++++++++++++++-------------- src/qemu/qemu_hotplug.h | 33 +++-- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 ++- src/remote_protocol-structs | 6 + tests/qemuhotplugtest.c | 13 +- tools/virsh-domain.c | 72 ++++++++++ tools/virsh.pod | 12 ++ 14 files changed, 525 insertions(+), 143 deletions(-) -- 2.16.1
When detaching a device it can be uniquely identified by its alias. Instead of misusing virDomainDetachDeviceFlags which has the same signature introduce new function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 3 +++ src/driver-hypervisor.h | 6 +++++ src/libvirt-domain.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 67 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index d7cbd18796..da773b76cb 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2022,6 +2022,9 @@ int virDomainDetachDeviceFlags(virDomainPtr domain, int virDomainUpdateDeviceFlags(virDomainPtr domain, const char *xml, unsigned int flags); +int virDomainDetachDeviceAlias(virDomainPtr domain, + const char *alias, unsigned int flags); + typedef struct _virDomainStatsRecord virDomainStatsRecord; typedef virDomainStatsRecord *virDomainStatsRecordPtr; struct _virDomainStatsRecord { diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index e71a72a441..9a224998fd 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -441,6 +441,11 @@ typedef int const char *xml, unsigned int flags); +typedef int +(*virDrvDomainDetachDeviceAlias)(virDomainPtr domain, + const char *alias, + unsigned int flags); + typedef int (*virDrvDomainGetAutostart)(virDomainPtr domain, int *autostart); @@ -1392,6 +1397,7 @@ struct _virHypervisorDriver { virDrvDomainDetachDevice domainDetachDevice; virDrvDomainDetachDeviceFlags domainDetachDeviceFlags; virDrvDomainUpdateDeviceFlags domainUpdateDeviceFlags; + virDrvDomainDetachDeviceAlias domainDetachDeviceAlias; virDrvDomainGetAutostart domainGetAutostart; virDrvDomainSetAutostart domainSetAutostart; virDrvDomainGetSchedulerType domainGetSchedulerType; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 2d86e48979..e536781e38 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8349,6 +8349,59 @@ virDomainUpdateDeviceFlags(virDomainPtr domain, } +/** + * virDomainDetachDeviceAlias: + * @domain: pointer to domain object + * @alias: device alias + * @flags: bitwise-OR of virDomainDeviceModifyFlags + * + * Detach a virtual device from a domain, using the alias to + * specify device. The value of @flags should be either + * VIR_DOMAIN_AFFECT_CURRENT, or a bitwise-or of values from + * VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CURRENT, although + * hypervisors vary in which flags are supported. + * + * In contrast to virDomainDetachDeviceFlags() this API only send + * request to the hypervisor and returns immediately. It's + * caller's responsibility to wait for + * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event to signal actual + * device removal. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainDetachDeviceAlias(virDomainPtr domain, + const char *alias, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "alias=%s, flags=0x%x", alias, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + conn = domain->conn; + + virCheckNonNullArgGoto(alias, error); + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->driver->domainDetachDeviceAlias) { + int ret; + ret = conn->driver->domainDetachDeviceAlias(domain, alias, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} + + /** * virConnectDomainEventRegister: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 95df3a0dbc..cd6b0c1fdc 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -785,4 +785,9 @@ LIBVIRT_4.1.0 { virStoragePoolLookupByTargetPath; } LIBVIRT_3.9.0; +LIBVIRT_4.4.0 { + global: + virDomainDetachDeviceAlias; +} LIBVIRT_4.1.0; + # .... define new API here using predicted next version number .... -- 2.16.1
On Thu, May 24, 2018 at 01:13:28PM +0200, Michal Privoznik wrote:
When detaching a device it can be uniquely identified by its alias. Instead of misusing virDomainDetachDeviceFlags which has the same signature introduce new function.
s/new/a new/
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 3 +++ src/driver-hypervisor.h | 6 +++++ src/libvirt-domain.c | 53 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 67 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 2d86e48979..e536781e38 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8349,6 +8349,59 @@ virDomainUpdateDeviceFlags(virDomainPtr domain, }
+/** + * virDomainDetachDeviceAlias: + * @domain: pointer to domain object
s/domain/a domain/ or even d/pointer to /
+ * @alias: device alias + * @flags: bitwise-OR of virDomainDeviceModifyFlags + * + * Detach a virtual device from a domain, using the alias to + * specify device. The value of @flags should be either
s/device/the device/
+ * VIR_DOMAIN_AFFECT_CURRENT, or a bitwise-or of values from + * VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CURRENT, although + * hypervisors vary in which flags are supported. + * + * In contrast to virDomainDetachDeviceFlags() this API only send + * request to the hypervisor and returns immediately.
this API is asynchronous - it returns immediately after sending the detach request to the hypervisor.
It's + * caller's responsibility to wait for
s/caller/the caller/
+ * VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event to signal actual + * device removal. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainDetachDeviceAlias(virDomainPtr domain, + const char *alias, + unsigned int flags)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +++++++++++++++- src/remote_protocol-structs | 6 ++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 95437b4365..8695046b71 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8308,6 +8308,7 @@ static virHypervisorDriver hypervisor_driver = { .domainDetachDevice = remoteDomainDetachDevice, /* 0.3.0 */ .domainDetachDeviceFlags = remoteDomainDetachDeviceFlags, /* 0.7.7 */ .domainUpdateDeviceFlags = remoteDomainUpdateDeviceFlags, /* 0.8.0 */ + .domainDetachDeviceAlias = remoteDomainDetachDeviceAlias, /* 4.4.0 */ .domainGetAutostart = remoteDomainGetAutostart, /* 0.3.0 */ .domainSetAutostart = remoteDomainSetAutostart, /* 0.3.0 */ .domainGetSchedulerType = remoteDomainGetSchedulerType, /* 0.3.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 296a087181..b23c93514a 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1284,6 +1284,12 @@ struct remote_domain_update_device_flags_args { unsigned int flags; }; +struct remote_domain_detach_device_alias_args { + remote_nonnull_domain dom; + remote_nonnull_string alias; + unsigned int flags; +}; + struct remote_domain_get_autostart_args { remote_nonnull_domain dom; }; @@ -6135,5 +6141,13 @@ enum remote_procedure { * @priority: high * @acl: storage_pool:getattr */ - REMOTE_PROC_STORAGE_POOL_LOOKUP_BY_TARGET_PATH = 391 + REMOTE_PROC_STORAGE_POOL_LOOKUP_BY_TARGET_PATH = 391, + + /** + * @generate: both + * @acl: domain:write + * @acl: domain:save:!VIR_DOMAIN_AFFECT_CONFIG|VIR_DOMAIN_AFFECT_LIVE + * @acl: domain:save:VIR_DOMAIN_AFFECT_CONFIG + */ + REMOTE_PROC_DOMAIN_DETACH_DEVICE_ALIAS = 392 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index fe163db73f..1b4fbc5d4b 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -915,6 +915,11 @@ struct remote_domain_update_device_flags_args { remote_nonnull_string xml; u_int flags; }; +struct remote_domain_detach_device_alias_args { + remote_nonnull_domain dom; + remote_nonnull_string alias; + u_int flags; +}; struct remote_domain_get_autostart_args { remote_nonnull_domain dom; }; @@ -3269,4 +3274,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_MANAGED_SAVE_DEFINE_XML = 389, REMOTE_PROC_DOMAIN_SET_LIFECYCLE_ACTION = 390, REMOTE_PROC_STORAGE_POOL_LOOKUP_BY_TARGET_PATH = 391, + REMOTE_PROC_DOMAIN_DETACH_DEVICE_ALIAS = 392, }; -- 2.16.1
On Thu, May 24, 2018 at 01:13:29PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +++++++++++++++- src/remote_protocol-structs | 6 ++++++ 3 files changed, 22 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 12 +++++++++ 2 files changed, 84 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cfbbf5a7bc..26f7983540 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11878,6 +11878,72 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) return funcRet; } + +/* + * "detach-device-alias" command + */ +static const vshCmdInfo info_detach_device_alias[] = { + {.name = "help", + .data = N_("detach device from an alias") + }, + {.name = "desc", + .data = N_("Detach device from domain using given alias to identify device") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_detach_device_alias[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "alias", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("device alias") + }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = NULL} +}; + +static bool +cmdDetachDeviceAlias(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *alias = NULL; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool ret = false; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "alias", &alias) < 0) + goto cleanup; + + if (virDomainDetachDeviceAlias(dom, alias, flags) < 0) { + vshError(ctl, _("Failed to detach device with alias %s"), alias); + goto cleanup; + } + + vshPrintExtra(ctl, "%s", _("Device detached successfully\n")); + ret = true; + + cleanup: + virshDomainFree(dom); + return ret; +} + + /* * "update-device" command */ @@ -13999,6 +14065,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_detach_device, .flags = 0 }, + {.name = "detach-device-alias", + .handler = cmdDetachDeviceAlias, + .opts = opts_detach_device_alias, + .info = info_detach_device_alias, + .flags = 0 + }, {.name = "detach-disk", .handler = cmdDetachDisk, .opts = opts_detach_disk, diff --git a/tools/virsh.pod b/tools/virsh.pod index 929958a953..45c1a3f271 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3112,6 +3112,18 @@ an offline domain, and like I<--live> I<--config> for a running domain. Note that older versions of virsh used I<--config> as an alias for I<--persistent>. +=item B<detach-device-alias> I<domain> I<alias> +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] + +Detach a device with given I<alias> from the I<domain>. + +If I<--live> is specified, affect a running domain. +If I<--config> is specified, affect the next startup of a persistent domain. +If I<--current> is specified, affect the current domain state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. When no flag is specified legacy API is used whose behavior depends +on the hypervisor driver. + =item B<detach-disk> I<domain> I<target> [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] [I<--print-xml>] -- 2.16.1
On Thu, May 24, 2018 at 01:13:30PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 12 +++++++++ 2 files changed, 84 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cfbbf5a7bc..26f7983540 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11878,6 +11878,72 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) return funcRet; }
+ +/* + * "detach-device-alias" command + */ +static const vshCmdInfo info_detach_device_alias[] = { + {.name = "help", + .data = N_("detach device from an alias") + }, + {.name = "desc", + .data = N_("Detach device from domain using given alias to identify device")
"from a domain" feels better. Or even: Detach device identified by the given alias from a domain
+ }, + {.name = NULL} +}; +
[...]
+static bool +cmdDetachDeviceAlias(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *alias = NULL; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool ret = false; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "alias", &alias) < 0) + goto cleanup; + + if (virDomainDetachDeviceAlias(dom, alias, flags) < 0) { + vshError(ctl, _("Failed to detach device with alias %s"), alias); + goto cleanup; + } + + vshPrintExtra(ctl, "%s", _("Device detached successfully\n"));
Or, to match the semantics of the API more closely: "Device detach request sent successfully\n"
+ ret = true; + + cleanup: + virshDomainFree(dom); + return ret; +} + + /* * "update-device" command */ @@ -13999,6 +14065,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_detach_device, .flags = 0 }, + {.name = "detach-device-alias", + .handler = cmdDetachDeviceAlias, + .opts = opts_detach_device_alias, + .info = info_detach_device_alias, + .flags = 0 + }, {.name = "detach-disk", .handler = cmdDetachDisk, .opts = opts_detach_disk, diff --git a/tools/virsh.pod b/tools/virsh.pod index 929958a953..45c1a3f271 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3112,6 +3112,18 @@ an offline domain, and like I<--live> I<--config> for a running domain. Note that older versions of virsh used I<--config> as an alias for I<--persistent>.
+=item B<detach-device-alias> I<domain> I<alias> +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] + +Detach a device with given I<alias> from the I<domain>. + +If I<--live> is specified, affect a running domain. +If I<--config> is specified, affect the next startup of a persistent domain. +If I<--current> is specified, affect the current domain state. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive.
When no flag is specified legacy API is used whose behavior depends +on the hypervisor driver. +
Drop this sentence, it is not relevant for the new API. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Instead of releasing address only sometimes in qemuDomainDetachChrDevice() let's release it whenever the device is actually removed from the domain definition. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b35594be5f..cddd700af8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4337,6 +4337,7 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias); qemuDomainEventQueue(driver, event); + qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); qemuDomainChrRemove(vm->def, chr); virDomainChrDefFree(chr); ret = 0; @@ -5620,10 +5621,8 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { - qemuDomainReleaseDeviceAddress(vm, &tmpChr->info, NULL); + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); - } cleanup: qemuDomainResetDeviceRemoval(vm); -- 2.16.1
On Thu, May 24, 2018 at 01:13:31PM +0200, Michal Privoznik wrote:
Instead of releasing address only sometimes in qemuDomainDetachChrDevice() let's release it whenever the device is actually removed from the domain definition.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On shmem unplug, when qemu doesn't support DEVICE_DELETED event (or couple of other reasons) we do two things: 1) release shmem device address, 2) call qemuDomainRemoveShmemDevice() which does 1) again. This is potentially dangerous. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cddd700af8..dba4bc9a6e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5291,10 +5291,8 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, ret = -1; if (ret == 0) { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { - qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveShmemDevice(driver, vm, shmem); - } } qemuDomainResetDeviceRemoval(vm); -- 2.16.1
On Thu, May 24, 2018 at 01:13:32PM +0200, Michal Privoznik wrote:
On shmem unplug, when qemu doesn't support DEVICE_DELETED event (or couple of other reasons) we do two things:
1) release shmem device address, 2) call qemuDomainRemoveShmemDevice() which does 1) again.
This is potentially dangerous.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On watchdog unplug, when qemu doesn't support DEVICE_DELETED event (or couple of other reasons) we do two things: 1) release watchdog device address, 2) call qemuDomainRemoveWatchdog() which does 1) again. This is potentially dangerous. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index dba4bc9a6e..0062e27a3b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5344,10 +5344,8 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, ret = -1; if (ret == 0) { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { - qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL); + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) ret = qemuDomainRemoveWatchdog(driver, vm, watchdog); - } } qemuDomainResetDeviceRemoval(vm); -- 2.16.1
On Thu, May 24, 2018 at 01:13:33PM +0200, Michal Privoznik wrote:
On watchdog unplug, when qemu doesn't support DEVICE_DELETED event (or couple of other reasons) we do two things:
1) release watchdog device address, 2) call qemuDomainRemoveWatchdog() which does 1) again.
This is potentially dangerous.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We are overwriting @ret a lot. It makes hard to see what is actually going on. Use more gotos. Two functions are fixed here: qemuDomainDetachShmemDevice() and qemuDomainDetachWatchdog(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0062e27a3b..e84dc909b9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5283,19 +5283,20 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, } qemuDomainMarkDeviceForRemoval(vm, &shmem->info); + qemuDomainObjEnterMonitor(driver, vm); - - ret = qemuMonitorDelDevice(priv->mon, shmem->info.alias); - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; - - if (ret == 0) { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveShmemDevice(driver, vm, shmem); + if (qemuMonitorDelDevice(priv->mon, shmem->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveShmemDevice(driver, vm, shmem); + + cleanup: qemuDomainResetDeviceRemoval(vm); - return ret; } @@ -5336,19 +5337,20 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, } qemuDomainMarkDeviceForRemoval(vm, &watchdog->info); + qemuDomainObjEnterMonitor(driver, vm); - - ret = qemuMonitorDelDevice(priv->mon, watchdog->info.alias); - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; - - if (ret == 0) { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveWatchdog(driver, vm, watchdog); + if (qemuMonitorDelDevice(priv->mon, watchdog->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveWatchdog(driver, vm, watchdog); + + cleanup: qemuDomainResetDeviceRemoval(vm); - return ret; } -- 2.16.1
On Thu, May 24, 2018 at 01:13:34PM +0200, Michal Privoznik wrote:
We are overwriting @ret a lot. It makes hard to see what is actually going on. Use more gotos. Two functions are fixed here: qemuDomainDetachShmemDevice() and qemuDomainDetachWatchdog().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Funny, we obtain driver caps at the beginning of the function, but then for unknown reason access driver->caps directly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3a328e5d46..992f140b2b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8723,7 +8723,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, * changed even if we failed to attach the device. For example, * a new controller may be created. */ - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) { + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, caps) < 0) { ret = -1; goto cleanup; } @@ -8731,7 +8731,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - ret = virDomainSaveConfig(cfg->configDir, driver->caps, vmdef); + ret = virDomainSaveConfig(cfg->configDir, caps, vmdef); if (!ret) { virDomainObjAssignDef(vm, vmdef, false, NULL); vmdef = NULL; -- 2.16.1
On Thu, May 24, 2018 at 01:13:35PM +0200, Michal Privoznik wrote:
Funny, we obtain driver caps at the beginning of the function, but then for unknown reason access driver->caps directly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The fact that we are overwriting @ret multiple times is very confusing to see what is actually happening here. Follow our traditional pattern where @ret is initialized to -1, and set to 0 only in case we know we succeeded. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 992f140b2b..2347e71cfb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8709,35 +8709,35 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, if (!vmdef) goto cleanup; - if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev, caps, - parse_flags, - driver->xmlopt)) < 0) + if (qemuDomainDetachDeviceConfig(vmdef, dev, caps, + parse_flags, + driver->xmlopt) < 0) goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, driver)) < 0) + if (qemuDomainDetachDeviceLive(vm, dev_copy, driver) < 0) goto cleanup; /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, * a new controller may be created. */ - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, caps) < 0) { - ret = -1; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, caps) < 0) goto cleanup; - } } /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - ret = virDomainSaveConfig(cfg->configDir, caps, vmdef); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + if (virDomainSaveConfig(cfg->configDir, caps, vmdef) < 0) + goto cleanup; + + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; } + ret = 0; + cleanup: virObjectUnref(caps); virObjectUnref(cfg); -- 2.16.1
On Thu, May 24, 2018 at 01:13:36PM +0200, Michal Privoznik wrote:
The fact that we are overwriting @ret multiple times is very confusing to see what is actually happening here.
-EPARSE s/is very confusing/makes it difficult to see/
Follow our traditional pattern where @ret is initialized to -1, and set to 0 only in case we know we succeeded.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The virDomainDetachDeviceAlias API is designed so that it only sends detach request to qemu. It's user's responsibility to wait for DEVICE_DELETED event, not libvirt's. Add @async flag to qemuDomainDetach*Device() functions so that caller can chose if detach is semi-synchronous (old virDomainDetachDeviceFlags()) or fully asynchronous (new virDomainDetachDeviceFlags()). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 32 +++---- src/qemu/qemu_hotplug.c | 231 ++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_hotplug.h | 33 ++++--- tests/qemuhotplugtest.c | 13 +-- 4 files changed, 203 insertions(+), 106 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2347e71cfb..81a9833b39 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7726,14 +7726,15 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, static int qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + 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); + ret = qemuDomainDetachControllerDevice(driver, vm, dev, async); break; default : virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -7746,46 +7747,47 @@ qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver, static int qemuDomainDetachDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev, - virQEMUDriverPtr driver) + virQEMUDriverPtr driver, + bool async) { int ret = -1; switch ((virDomainDeviceType)dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev); + ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async); break; case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev); + 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); + ret = qemuDomainDetachNetDevice(driver, vm, dev, async); break; case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = qemuDomainDetachHostDevice(driver, vm, dev); + ret = qemuDomainDetachHostDevice(driver, vm, dev, async); break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); + ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr, async); break; case VIR_DOMAIN_DEVICE_RNG: - ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng); + ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng, async); break; case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory); + ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory, async); break; case VIR_DOMAIN_DEVICE_SHMEM: - ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem); + ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem, async); break; case VIR_DOMAIN_DEVICE_WATCHDOG: - ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog); + ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog, async); break; case VIR_DOMAIN_DEVICE_INPUT: - ret = qemuDomainDetachInputDevice(vm, dev->data.input); + ret = qemuDomainDetachInputDevice(vm, dev->data.input, async); break; case VIR_DOMAIN_DEVICE_REDIRDEV: - ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev); + ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev, async); break; case VIR_DOMAIN_DEVICE_FS: @@ -8716,7 +8718,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (qemuDomainDetachDeviceLive(vm, dev_copy, driver) < 0) + if (qemuDomainDetachDeviceLive(vm, dev_copy, driver, false) < 0) goto cleanup; /* * update domain status forcibly because the domain status may be diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e84dc909b9..5b5c27b011 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4740,7 +4740,8 @@ qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, static int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr detach) + virDomainDiskDefPtr detach, + bool async) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -4757,7 +4758,8 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, goto cleanup; } - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { @@ -4769,18 +4771,24 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } static int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr detach) + virDomainDiskDefPtr detach, + bool async) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -4788,7 +4796,8 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, if (qemuDomainDiskBlockJobIsActive(detach)) goto cleanup; - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { @@ -4800,11 +4809,16 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } @@ -4824,7 +4838,8 @@ qemuFindDisk(virDomainDefPtr def, const char *dst) int qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + bool async) { virDomainDiskDefPtr disk; int ret = -1; @@ -4841,10 +4856,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) - ret = qemuDomainDetachVirtioDiskDevice(driver, vm, disk); + ret = qemuDomainDetachVirtioDiskDevice(driver, vm, disk, async); else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || disk->bus == VIR_DOMAIN_DISK_BUS_USB) - ret = qemuDomainDetachDiskDevice(driver, vm, disk); + ret = qemuDomainDetachDiskDevice(driver, vm, disk, async); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("This type of disk cannot be hot unplugged")); @@ -4922,7 +4937,8 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm, int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + bool async) { int idx, ret = -1; virDomainControllerDefPtr detach = NULL; @@ -4974,7 +4990,8 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, goto cleanup; } - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) { @@ -4984,18 +5001,24 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveControllerDevice(driver, vm, detach); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveControllerDevice(driver, vm, detach); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } static int qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainHostdevDefPtr detach) + virDomainHostdevDefPtr detach, + bool async) { qemuDomainObjPrivatePtr priv = vm->privateData; virDomainHostdevSubsysPCIPtr pcisrc = &detach->source.subsys.u.pci; @@ -5009,7 +5032,8 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, detach->info); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); @@ -5022,7 +5046,8 @@ qemuDomainDetachHostPCIDevice(virQEMUDriverPtr driver, static int qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainHostdevDefPtr detach) + virDomainHostdevDefPtr detach, + bool async) { qemuDomainObjPrivatePtr priv = vm->privateData; int ret; @@ -5033,7 +5058,8 @@ qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, detach->info); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); @@ -5046,7 +5072,8 @@ qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver, static int qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainHostdevDefPtr detach) + virDomainHostdevDefPtr detach, + bool async) { qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; @@ -5057,7 +5084,8 @@ qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, detach->info); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); @@ -5071,7 +5099,8 @@ qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver, static int qemuDomainDetachSCSIVHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainHostdevDefPtr detach) + virDomainHostdevDefPtr detach, + bool async) { qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; @@ -5082,7 +5111,8 @@ qemuDomainDetachSCSIVHostDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, detach->info); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); @@ -5097,7 +5127,8 @@ qemuDomainDetachSCSIVHostDevice(virQEMUDriverPtr driver, static int qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainHostdevDefPtr detach) + virDomainHostdevDefPtr detach, + bool async) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -5108,7 +5139,8 @@ qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, detach->info); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); @@ -5122,7 +5154,8 @@ qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver, static int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainHostdevDefPtr detach) + virDomainHostdevDefPtr detach, + bool async) { int ret = -1; @@ -5131,19 +5164,19 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, switch (detach->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - ret = qemuDomainDetachHostPCIDevice(driver, vm, detach); + ret = qemuDomainDetachHostPCIDevice(driver, vm, detach, async); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - ret = qemuDomainDetachHostUSBDevice(driver, vm, detach); + ret = qemuDomainDetachHostUSBDevice(driver, vm, detach, async); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: - ret = qemuDomainDetachHostSCSIDevice(driver, vm, detach); + ret = qemuDomainDetachHostSCSIDevice(driver, vm, detach, async); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: - ret = qemuDomainDetachSCSIVHostDevice(driver, vm, detach); + ret = qemuDomainDetachSCSIVHostDevice(driver, vm, detach, async); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: - ret = qemuDomainDetachMediatedDevice(driver, vm, detach); + ret = qemuDomainDetachMediatedDevice(driver, vm, detach, async); break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -5155,11 +5188,13 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, if (ret < 0) { if (virDomainObjIsActive(vm)) virDomainAuditHostdev(vm, detach, "detach", false); - } else if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { + } else if (!async && + (ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { ret = qemuDomainRemoveHostDevice(driver, vm, detach); } - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } @@ -5167,7 +5202,8 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, /* search for a hostdev matching dev and detach it */ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + bool async) { virDomainHostdevDefPtr hostdev = dev->data.hostdev; virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; @@ -5242,16 +5278,17 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, * function so that mac address / virtualport are reset */ if (detach->parent.type == VIR_DOMAIN_DEVICE_NET) - return qemuDomainDetachNetDevice(driver, vm, &detach->parent); + return qemuDomainDetachNetDevice(driver, vm, &detach->parent, async); else - return qemuDomainDetachThisHostDevice(driver, vm, detach); + return qemuDomainDetachThisHostDevice(driver, vm, detach, async); } int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainShmemDefPtr dev) + virDomainShmemDefPtr dev, + bool async) { int ret = -1; ssize_t idx = -1; @@ -5282,7 +5319,8 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, &shmem->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &shmem->info); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, shmem->info.alias) < 0) { @@ -5304,7 +5342,8 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, int qemuDomainDetachWatchdog(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainWatchdogDefPtr dev) + virDomainWatchdogDefPtr dev, + bool async) { int ret = -1; virDomainWatchdogDefPtr watchdog = vm->def->watchdog; @@ -5336,7 +5375,8 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, &watchdog->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &watchdog->info); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, watchdog->info.alias) < 0) { @@ -5358,7 +5398,8 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainRedirdevDefPtr dev) + virDomainRedirdevDefPtr dev, + bool async) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -5379,7 +5420,8 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdevDef->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdevDef->info); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, tmpRedirdevDef->info.alias) < 0) { @@ -5389,11 +5431,16 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdevDef); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdevDef); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } @@ -5401,7 +5448,8 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + bool async) { int detachidx, ret = -1; virDomainNetDefPtr detach = NULL; @@ -5414,7 +5462,8 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { ret = qemuDomainDetachThisHostDevice(driver, vm, - virDomainNetGetActualHostdev(detach)); + virDomainNetGetActualHostdev(detach),\ + async); goto cleanup; } @@ -5442,7 +5491,8 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, */ ignore_value(qemuInterfaceStopDevice(detach)); - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &detach->info); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { @@ -5454,11 +5504,16 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveNetDevice(driver, vm, detach); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveNetDevice(driver, vm, detach); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } @@ -5585,7 +5640,8 @@ int qemuDomainDetachLease(virQEMUDriverPtr driver, int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainChrDefPtr chr) + virDomainChrDefPtr chr, + bool async) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -5609,7 +5665,8 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup; - qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info); qemuDomainObjEnterMonitor(driver, vm); if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { @@ -5619,11 +5676,16 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); return ret; } @@ -5632,7 +5694,8 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainRNGDefPtr rng) + virDomainRNGDefPtr rng, + bool async) { qemuDomainObjPrivatePtr priv = vm->privateData; ssize_t idx; @@ -5656,18 +5719,24 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info); qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorDelDevice(priv->mon, tmpRNG->info.alias); if (qemuDomainObjExitMonitor(driver, vm) || rc < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } @@ -5675,7 +5744,8 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, int qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainMemoryDefPtr memdef) + virDomainMemoryDefPtr memdef, + bool async) { qemuDomainObjPrivatePtr priv = vm->privateData; virDomainMemoryDefPtr mem; @@ -5701,18 +5771,24 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, return -1; } - qemuDomainMarkDeviceForRemoval(vm, &mem->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &mem->info); qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorDelDevice(priv->mon, mem->info.alias); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveMemoryDevice(driver, vm, mem); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveMemoryDevice(driver, vm, mem); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } @@ -6380,7 +6456,8 @@ qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, int qemuDomainDetachInputDevice(virDomainObjPtr vm, - virDomainInputDefPtr def) + virDomainInputDefPtr def, + bool async) { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; @@ -6410,7 +6487,8 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm, break; } - qemuDomainMarkDeviceForRemoval(vm, &input->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &input->info); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, input->info.alias)) { @@ -6420,10 +6498,15 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveInputDevice(vm, input); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveInputDevice(vm, input); + } cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index b2f5fa688b..751cbf61d4 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -87,7 +87,8 @@ int qemuDomainAttachMemory(virQEMUDriverPtr driver, virDomainMemoryDefPtr mem); int qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainMemoryDefPtr memdef); + virDomainMemoryDefPtr memdef, + bool async); int qemuDomainChangeGraphics(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainGraphicsDefPtr dev); @@ -106,26 +107,33 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver, int linkstate); int qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev); + virDomainDeviceDefPtr dev, + bool async); int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev); + virDomainDeviceDefPtr dev, + bool async); int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev); + virDomainDeviceDefPtr dev, + bool async); int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev); + virDomainDeviceDefPtr dev, + bool async); int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainShmemDefPtr dev); + virDomainShmemDefPtr dev, + bool async); int qemuDomainDetachWatchdog(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainWatchdogDefPtr watchdog); + virDomainWatchdogDefPtr watchdog, + bool async); int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainRedirdevDefPtr dev); + virDomainRedirdevDefPtr dev, + bool async); int qemuDomainAttachInputDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -142,13 +150,15 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr chr); int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainChrDefPtr chr); + virDomainChrDefPtr chr, + bool async); int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng); int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainRNGDefPtr rng); + virDomainRNGDefPtr rng, + bool async); void qemuDomainRemoveVcpuAlias(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -184,6 +194,7 @@ int qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, bool state); int qemuDomainDetachInputDevice(virDomainObjPtr vm, - virDomainInputDefPtr def); + virDomainInputDefPtr def, + bool async); #endif /* __QEMU_HOTPLUG_H__ */ diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index e93503812a..663e33ed00 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -142,22 +142,23 @@ testQemuHotplugAttach(virDomainObjPtr vm, static int testQemuHotplugDetach(virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + bool async) { int ret = -1; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainDetachDeviceDiskLive(&driver, vm, dev); + ret = qemuDomainDetachDeviceDiskLive(&driver, vm, dev, async); break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainDetachChrDevice(&driver, vm, dev->data.chr); + ret = qemuDomainDetachChrDevice(&driver, vm, dev->data.chr, async); break; case VIR_DOMAIN_DEVICE_SHMEM: - ret = qemuDomainDetachShmemDevice(&driver, vm, dev->data.shmem); + ret = qemuDomainDetachShmemDevice(&driver, vm, dev->data.shmem, async); break; case VIR_DOMAIN_DEVICE_WATCHDOG: - ret = qemuDomainDetachWatchdog(&driver, vm, dev->data.watchdog); + ret = qemuDomainDetachWatchdog(&driver, vm, dev->data.watchdog, async); break; default: VIR_TEST_VERBOSE("device type '%s' cannot be detached\n", @@ -322,7 +323,7 @@ testQemuHotplug(const void *data) break; case DETACH: - ret = testQemuHotplugDetach(vm, dev); + ret = testQemuHotplugDetach(vm, dev, false); if (ret == 0 || fail) ret = testQemuHotplugCheckResult(vm, domain_xml, domain_filename, fail); -- 2.16.1
On Thu, May 24, 2018 at 01:13:37PM +0200, Michal Privoznik wrote:
The virDomainDetachDeviceAlias API is designed so that it only sends detach request to qemu. It's user's responsibility to wait
s/user/the user/
for DEVICE_DELETED event, not libvirt's. Add @async flag to qemuDomainDetach*Device() functions so that caller can chose if detach is semi-synchronous (old virDomainDetachDeviceFlags()) or fully asynchronous (new virDomainDetachDeviceFlags()).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 32 +++---- src/qemu/qemu_hotplug.c | 231 ++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_hotplug.h | 33 ++++--- tests/qemuhotplugtest.c | 13 +-- 4 files changed, 203 insertions(+), 106 deletions(-)
@@ -4769,18 +4771,24 @@ qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
- if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + }
I'm sure the ret assignments could be reduced here, but that's out of scope of this series... Also, most of the code is executed if (!async). It might be nicer to come up with a variable that can be used in a positive condition, but it would be less precise, because only the new async API is reasonably defined - even if (wait) does not seem to outweigh the loss of clarity.
cleanup: - qemuDomainResetDeviceRemoval(vm); + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; }
static int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr detach) + virDomainDiskDefPtr detach, + bool async) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -5336,7 +5375,8 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, return -1; }
- qemuDomainMarkDeviceForRemoval(vm, &watchdog->info); + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &watchdog->info);
qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorDelDevice(priv->mon, watchdog->info.alias) < 0) {
Missing condition around qemuDomainWaitForDeviceRemoval here in qemuDomainDetachWatchdog. It should only be called if (!async). With that fixed: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, May 24, 2018 at 01:13:37PM +0200, Michal Privoznik wrote:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e84dc909b9..5b5c27b011 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5414,7 +5462,8 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) { ret = qemuDomainDetachThisHostDevice(driver, vm, - virDomainNetGetActualHostdev(detach)); + virDomainNetGetActualHostdev(detach),\
Spurious backslash. Jano
+ async); goto cleanup; }
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 81a9833b39..6f0a3d0cda 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8750,6 +8750,77 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, return ret; } + +static int +qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *alias, + unsigned int flags) +{ + virCapsPtr caps = NULL; + virQEMUDriverConfigPtr cfg = NULL; + virDomainDefPtr def = NULL; + virDomainDefPtr persistentDef = NULL; + virDomainDefPtr vmdef = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + cfg = virQEMUDriverGetConfig(driver); + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; + + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto cleanup; + + if (persistentDef) { + virDomainDeviceDef dev; + + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + if (!vmdef) + goto cleanup; + + if (virDomainDefFindDevice(persistentDef, alias, &dev, true) < 0) + goto cleanup; + + if (qemuDomainDetachDeviceConfig(persistentDef, &dev, caps, + parse_flags, driver->xmlopt) < 0) + goto cleanup; + } + + if (def) { + virDomainDeviceDef dev; + + if (virDomainDefFindDevice(def, alias, &dev, true) < 0) + goto cleanup; + + if (qemuDomainDetachDeviceLive(vm, &dev, driver, true) < 0) + goto cleanup; + } + + if (vmdef) { + if (virDomainSaveConfig(cfg->configDir, caps, vmdef) < 0) + goto cleanup; + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; + } + + ret = 0; + cleanup: + virDomainDefFree(vmdef); + virObjectUnref(cfg); + virObjectUnref(caps); + return ret; +} + + static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, @@ -8784,6 +8855,42 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, return ret; } + +static int +qemuDomainDetachDeviceAlias(virDomainPtr dom, + const char *alias, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainDetachDeviceAliasEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; + + if (qemuDomainDetachDeviceAliasLiveAndConfig(driver, vm, alias, flags) < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static int qemuDomainDetachDevice(virDomainPtr dom, const char *xml) { return qemuDomainDetachDeviceFlags(dom, xml, @@ -21279,6 +21386,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainDetachDevice = qemuDomainDetachDevice, /* 0.5.0 */ .domainDetachDeviceFlags = qemuDomainDetachDeviceFlags, /* 0.7.7 */ .domainUpdateDeviceFlags = qemuDomainUpdateDeviceFlags, /* 0.8.0 */ + .domainDetachDeviceAlias = qemuDomainDetachDeviceAlias, /* 4.4.0 */ .domainGetAutostart = qemuDomainGetAutostart, /* 0.2.1 */ .domainSetAutostart = qemuDomainSetAutostart, /* 0.2.1 */ .domainGetSchedulerType = qemuDomainGetSchedulerType, /* 0.7.0 */ -- 2.16.1
On Thu, May 24, 2018 at 01:13:38PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 81a9833b39..6f0a3d0cda 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8750,6 +8750,77 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, return ret; }
+ +static int +qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *alias, + unsigned int flags) +{ + virCapsPtr caps = NULL; + virQEMUDriverConfigPtr cfg = NULL; + virDomainDefPtr def = NULL; + virDomainDefPtr persistentDef = NULL; + virDomainDefPtr vmdef = NULL; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
Why on earth would we need parse_flags to detach a device?
+ int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + cfg = virQEMUDriverGetConfig(driver); + + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; + + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto cleanup; + + if (persistentDef) { + virDomainDeviceDef dev; + + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + if (!vmdef) + goto cleanup; + + if (virDomainDefFindDevice(persistentDef, alias, &dev, true) < 0) + goto cleanup; + + if (qemuDomainDetachDeviceConfig(persistentDef, &dev, caps, + parse_flags, driver->xmlopt) < 0)
Oh, somebody thought it would be a good idea to call postParse even though no parsing has happened.... Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/news.xml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 76d1613d35..5eb5902945 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -64,6 +64,14 @@ TLS environment which is setup for the migration connection. </description> </change> + <change> + <summary> + Introduce virDomainDetachDeviceAlias + </summary> + <description> + This new API enables users to detach device using only its alias. + </description> + </change> </section> <section title="Improvements"> <change> -- 2.16.1
On Thu, May 24, 2018 at 01:13:39PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/news.xml | 8 ++++++++ 1 file changed, 8 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik