On 9/4/25 14:10, Stefan Kober wrote:
> On-behalf-of: SAP [email protected]
> Signed-off-by: Stefan Kober <[email protected]>
> ---
> src/ch/ch_driver.c | 42 +++++++++++
> src/ch/ch_hotplug.c | 175 ++++++++++++++++++++++++++++++++++++++++++++
> src/ch/ch_hotplug.h | 6 ++
> 3 files changed, 223 insertions(+)
>
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 4f4783efb1..760fccba82 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -2387,6 +2387,46 @@ chDomainAttachDevice(virDomainPtr dom,
> return chDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE);
> }
>
> +static int
> +chDomainDetachDeviceFlags(virDomainPtr dom,
> + const char *xml,
> + unsigned int flags)
> +{
> + virCHDriver *driver = dom->conn->privateData;
> + virDomainObj *vm = NULL;
> + int ret = -1;
> +
> + if (!(vm = virCHDomainObjFromDomain(dom)))
> + goto cleanup;
> +
> + if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
> + goto cleanup;
> +
> + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> + goto endjob;
> +
> + if (chDomainDetachDeviceLiveAndUpdateConfig(driver, vm, xml, flags) < 0)
> + goto endjob;
> +
> + ret = 0;
> +
> + endjob:
> + virDomainObjEndJob(vm);
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
> +
> +static int chDomainDetachDevice(virDomainPtr dom, const char *xml)
> +{
> + return chDomainDetachDeviceFlags(dom, xml,
> + VIR_DOMAIN_AFFECT_LIVE);
> +}
> +
> /* Function Tables */
> static virHypervisorDriver chHypervisorDriver = {
> .name = "CH",
> @@ -2450,6 +2490,8 @@ static virHypervisorDriver chHypervisorDriver = {
> .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */
> .domainAttachDevice = chDomainAttachDevice, /* 11.8.0 */
> .domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.8.0 */
> + .domainDetachDevice = chDomainDetachDevice, /* 11.8.0 */
> + .domainDetachDeviceFlags = chDomainDetachDeviceFlags, /* 11.8.0 */
> };
>
> static virConnectDriver chConnectDriver = {
Again, until here the patch is fine and what's below should be moved
into a separate patch.
> diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c
> index 524355b93c..95fe1f0f6f 100644
> --- a/src/ch/ch_hotplug.c
> +++ b/src/ch/ch_hotplug.c
> @@ -156,3 +156,178 @@ chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj
> *vm,
>
> return 0;
> }
> +
> +static int
> +chFindDiskId(virDomainDef *def, const char *dst)
> +{
> + size_t i;
> +
> + for (i = 0; i < def->ndisks; i++) {
> + if (STREQ(def->disks[i]->dst, dst))
> + return i;
> + }
> +
> + return -1;
> +}
> +
> +
> +/**
> + * chDomainFindDisk
> + *
> + * Helper function to find a disk device definition of a domain.
> + *
> + * Searches through the disk devices of a domain by comparing to 'match' and
> + * returns any match via the 'detach' out parameter.
> + */
> +static int
> +chDomainFindDisk(virDomainObj *vm,
> + virDomainDiskDef *match,
> + virDomainDiskDef **detach)
> +{
> + virDomainDiskDef *disk;
> + int idx;
> +
> + if ((idx = chFindDiskId(vm->def, match->dst)) < 0) {
> + virReportError(VIR_ERR_DEVICE_MISSING,
> + _("disk %1$s not found"), match->dst);
> + return -1;
> + }
> + *detach = disk = vm->def->disks[idx];
> +
> + return 0;
> +}
> +
> +static int
> +chDomainDetachDeviceLive(virDomainObj *vm,
> + virDomainDeviceDef *match)
> +{
> + virDomainDeviceDef detach = { .type = match->type };
> + virDomainDeviceInfo *info = NULL;
> + virCHDomainObjPrivate *priv = vm->privateData;
> + int idx = 0;
> +
> + switch (match->type) {
> + case VIR_DOMAIN_DEVICE_DISK:
> + if (chDomainFindDisk(vm, match->data.disk,
> + &detach.data.disk) < 0) {
> + return -1;
> + }
> + break;
> + case VIR_DOMAIN_DEVICE_LEASE:
> + case VIR_DOMAIN_DEVICE_FS:
> + case VIR_DOMAIN_DEVICE_NET:
> + case VIR_DOMAIN_DEVICE_INPUT:
> + case VIR_DOMAIN_DEVICE_SOUND:
> + case VIR_DOMAIN_DEVICE_VIDEO:
> + case VIR_DOMAIN_DEVICE_HOSTDEV:
> + case VIR_DOMAIN_DEVICE_WATCHDOG:
> + case VIR_DOMAIN_DEVICE_CONTROLLER:
> + case VIR_DOMAIN_DEVICE_GRAPHICS:
> + case VIR_DOMAIN_DEVICE_HUB:
> + case VIR_DOMAIN_DEVICE_REDIRDEV:
> + case VIR_DOMAIN_DEVICE_SMARTCARD:
> + case VIR_DOMAIN_DEVICE_CHR:
> + case VIR_DOMAIN_DEVICE_MEMBALLOON:
> + case VIR_DOMAIN_DEVICE_NVRAM:
> + case VIR_DOMAIN_DEVICE_RNG:
> + case VIR_DOMAIN_DEVICE_SHMEM:
> + case VIR_DOMAIN_DEVICE_TPM:
> + case VIR_DOMAIN_DEVICE_PANIC:
> + case VIR_DOMAIN_DEVICE_MEMORY:
> + case VIR_DOMAIN_DEVICE_IOMMU:
> + case VIR_DOMAIN_DEVICE_VSOCK:
> + case VIR_DOMAIN_DEVICE_AUDIO:
> + case VIR_DOMAIN_DEVICE_CRYPTO:
> + case VIR_DOMAIN_DEVICE_PSTORE:
> + case VIR_DOMAIN_DEVICE_LAST:
> + case VIR_DOMAIN_DEVICE_NONE:
> + default:
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("live detach of device '%1$s' is not supported"),
> + virDomainDeviceTypeToString(match->type));
> + return -1;
> + }
> +
> + /* "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 '%1$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 %1$s device with no alias"),
> + virDomainDeviceTypeToString(detach.type));
> + return -1;
> + }
> +
> + if (virCHMonitorRemoveDevice(priv->monitor, info->alias) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Invalid response from CH. Disk removal failed."));
> + return -1;
> + }
> +
> + if (match->type == VIR_DOMAIN_DEVICE_DISK) {
> + idx = chFindDiskId(vm->def, match->data.disk->dst);
> + if (idx >= 0) {
> + virDomainDiskRemove(vm->def, idx);
> + }
> + }
Now, this little if() bothered me for a while. I think we need some
function, like chDomainDeviceRemove() with proper switch() over all
device types.
> +
> + return 0;
> +}
> +
> +int
> +chDomainDetachDeviceLiveAndUpdateConfig(virCHDriver *driver,
> + virDomainObj *vm,
> + const char *xml,
> + unsigned int flags)
> +{
> + g_autoptr(virCHDriverConfig) cfg = NULL;
> + g_autoptr(virDomainDeviceDef) dev_config = NULL;
> + g_autoptr(virDomainDeviceDef) dev_live = NULL;
> + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
> + g_autoptr(virDomainDef) vmdef = NULL;
> +
> + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> + VIR_DOMAIN_AFFECT_CONFIG, -1);
> +
> + cfg = virCHDriverGetConfig(driver);
> +
> + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) &&
> + !(flags & VIR_DOMAIN_AFFECT_LIVE))
> + parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
> +
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("Persistent domain state changes are not
> supported"));
> + return -1;
> + }
> +
> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> + if (!(dev_live = virDomainDeviceDefParse(xml, vm->def,
> driver->xmlopt,
> + NULL, parse_flags))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not parse domain definition"));
> + return -1;
> + }
> +
> + if (chDomainDetachDeviceLive(vm, dev_live) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could detach device"));
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> diff --git a/src/ch/ch_hotplug.h b/src/ch/ch_hotplug.h
> index 04915ba5de..4a9b9b3b3e 100644
> --- a/src/ch/ch_hotplug.h
> +++ b/src/ch/ch_hotplug.h
> @@ -25,3 +25,9 @@ chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm,
> virCHDriver *driver,
> const char *xml,
> unsigned int flags);
> +
> +int
> +chDomainDetachDeviceLiveAndUpdateConfig(virCHDriver *driver,
> + virDomainObj *vm,
> + const char *xml,
> + unsigned int flags);
Michal
