[libvirt] [PATCH v2 6/7] qemuDomainRemoveDevice: Enable live redirdev detach

[John Ferlan]

On 06/10/2016 11:33 AM, Michal Privoznik wrote:
> For some reason, as soon as redirdev is detached, qemu removes
> the chardev too. Well, it's easier for us then, so hard feelings
> here.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_driver.c  |  4 ++-
>  src/qemu/qemu_hotplug.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_hotplug.h |  3 ++
>  3 files changed, 81 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 254da59..65e96be 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7639,6 +7639,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>      case VIR_DOMAIN_DEVICE_MEMORY:
>          ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory);
>          break;
> +    case VIR_DOMAIN_DEVICE_REDIRDEV:
> +        ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev);
> +        break;
>  
>      case VIR_DOMAIN_DEVICE_FS:
>      case VIR_DOMAIN_DEVICE_INPUT:
> @@ -7651,7 +7654,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>      case VIR_DOMAIN_DEVICE_MEMBALLOON:
>      case VIR_DOMAIN_DEVICE_NVRAM:
>      case VIR_DOMAIN_DEVICE_SHMEM:
> -    case VIR_DOMAIN_DEVICE_REDIRDEV:
>      case VIR_DOMAIN_DEVICE_NONE:
>      case VIR_DOMAIN_DEVICE_TPM:
>      case VIR_DOMAIN_DEVICE_PANIC:
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 58156c6..a7a3675 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3230,6 +3230,34 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver,
>      return ret;
>  }
>  
> +static int
> +qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainRedirdevDefPtr redirdev)
> +{
> +    virObjectEventPtr event;
> +    ssize_t idx;
> +
> +    VIR_DEBUG("Removing redirdev device %s from domain %p %s",
> +              redirdev->info.alias, vm, vm->def->name);
> +
> +    if ((idx = virDomainRedirdevDefFind(vm->def, redirdev)) < 0)
> +        return 0;

This is a bit different than RNG processing... If for some reason this
doesn't exist, then the audit doesn't happen nor does the event.  It
would seem both would be wanted, wouldn't they?  Especially since we're
claiming success.

> +
> +    /* QEMU removed the chardev for us already. */

Will this always be true in the future?  Should we just try (like RNG)
and ignore the result.

> +
> +    virDomainAuditRedirdev(vm, redirdev, "detach", true);
> +
> +    event = virDomainEventDeviceRemovedNewFromObj(vm, redirdev->info.alias);
> +    qemuDomainEventQueue(driver, event);
> +
> +    virDomainRedirdevDefRemove(vm->def, idx);
> +    qemuDomainReleaseDeviceAddress(vm, &redirdev->info, NULL);
> +    virDomainRedirdevDefFree(redirdev);
> +    return 0;
> +}
> +
> +
>  
>  int
>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
> @@ -3261,6 +3289,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>      case VIR_DOMAIN_DEVICE_MEMORY:
>          ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory);
>          break;
> +    case VIR_DOMAIN_DEVICE_REDIRDEV:
> +        ret = qemuDomainRemoveRedirdevDevice(driver, vm, dev->data.redirdev);
> +        break;
>  
>      case VIR_DOMAIN_DEVICE_NONE:
>      case VIR_DOMAIN_DEVICE_LEASE:
> @@ -3271,7 +3302,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>      case VIR_DOMAIN_DEVICE_WATCHDOG:
>      case VIR_DOMAIN_DEVICE_GRAPHICS:
>      case VIR_DOMAIN_DEVICE_HUB:
> -    case VIR_DOMAIN_DEVICE_REDIRDEV:
>      case VIR_DOMAIN_DEVICE_SMARTCARD:
>      case VIR_DOMAIN_DEVICE_MEMBALLOON:
>      case VIR_DOMAIN_DEVICE_NVRAM:
> @@ -4172,3 +4202,47 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
>      qemuDomainResetDeviceRemoval(vm);
>      return ret;
>  }
> +
> +int
> +qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainRedirdevDefPtr redirdev)
> +{
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainRedirdevDefPtr tmpRedirdev;
> +    ssize_t idx;
> +
> +    if ((idx = virDomainRedirdevDefFind(vm->def, redirdev)) < 0) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("no matching redirdev was not found"));
> +        return -1;
> +    }
> +
> +    tmpRedirdev = vm->def->redirdevs[idx];
> +
> +    if (!tmpRedirdev->info.alias) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("alias for the redirdev was not found"));
> +        goto cleanup;
> +    }
> +
> +    qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdev->info);
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    if (qemuMonitorDelDevice(priv->mon, tmpRedirdev->info.alias) < 0) {
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        goto cleanup;
> +    }
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;
> +
> +    if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) {
> +        qemuDomainReleaseDeviceAddress(vm, &tmpRedirdev->info, NULL);

^^ This is called in RemoveRedirdevDevice, so it'd be duplicitous.

I think this is ACK-able with some obvious cleanups and perhaps thoughts
about the ordering in qemuDomainRemoveRedirdevDevice

John

> +        ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdev);
> +    }
> +
> + cleanup:
> +    qemuDomainResetDeviceRemoval(vm);
> +    return ret;
> +}
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 165d345..fc79ee5 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -46,6 +46,9 @@ int qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
>                                     virDomainObjPtr vm,
>                                     virDomainRedirdevDefPtr hostdev);
> +int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
> +                                   virDomainObjPtr vm,
> +                                   virDomainRedirdevDefPtr hostdev);
>  int qemuDomainAttachHostDevice(virConnectPtr conn,
>                                 virQEMUDriverPtr driver,
>                                 virDomainObjPtr vm,
>