[libvirt] [PATCH v2.1 1/2] qemu: Add support for hot unplug redirdev device

[John Ferlan]

On 12/07/2017 05:42 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at gmail.com>
> 
> We lacked of hot unplugging redirdev device.
> This patch add support for it.
> We could use detach-device --live now.

Alter to:

Add support for hot unplugging redirdev device which can use the
detach-device --live.

> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at gmail.com>
> ---
> v2.1:
>    rebase on master
> 
>  src/qemu/qemu_driver.c  |   4 +-
>  src/qemu/qemu_hotplug.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_hotplug.h |   3 ++
>  3 files changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index aa30b119a..180bd0ebf 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7787,6 +7787,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>      case VIR_DOMAIN_DEVICE_INPUT:
>          ret = qemuDomainDetachInputDevice(vm, dev->data.input);
>          break;
> +    case VIR_DOMAIN_DEVICE_REDIRDEV:
> +        ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev);
> +        break;
>  
>      case VIR_DOMAIN_DEVICE_FS:
>      case VIR_DOMAIN_DEVICE_SOUND:
> @@ -7796,7 +7799,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>      case VIR_DOMAIN_DEVICE_SMARTCARD:
>      case VIR_DOMAIN_DEVICE_MEMBALLOON:
>      case VIR_DOMAIN_DEVICE_NVRAM:
> -    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 24631cb8f..d97aa6051 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -4375,6 +4375,74 @@ qemuDomainRemoveInputDevice(virDomainObjPtr vm,
>      return 0;
>  }
>  

Two empty lines

> +static int
> +qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainRedirdevDefPtr redirdev)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    virObjectEventPtr event;
> +    char *charAlias = NULL;
> +    char *tlsAlias = NULL;
> +    char *secAlias = NULL;
> +    ssize_t idx;
> +    int ret = -1;
> +
> +    VIR_DEBUG("Removing redirdev device %s from domain %p %s",
> +              redirdev->info.alias, vm, vm->def->name);
> +
> +    if (!(charAlias = qemuAliasChardevFromDevAlias(redirdev->info.alias)))
> +        goto cleanup;
> +
> +    if (redirdev->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
> +        redirdev->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) {
> +
> +        if (!(tlsAlias = qemuAliasTLSObjFromSrcAlias(charAlias)))
> +            goto cleanup;
> +
> +        /* Best shot at this as the secinfo is destroyed after process launch
> +         * and this path does not recreate it. Thus, if the config has the
> +         * secret UUID and we have a serial TCP chardev, then formulate a
> +         * secAlias which we'll attempt to destroy. */
> +        if (cfg->chardevTLSx509secretUUID &&
> +            !(secAlias = qemuDomainGetSecretAESAlias(charAlias, false)))
> +            goto cleanup;
> +    }

Rather than cut-n-paste (again)... I've posted a patch that will
generate a qemuDomainDelChardevTLSObjects helper, see:

https://www.redhat.com/archives/libvir-list/2017-December/msg00678.html

That would change this code as well... I'll still make a few comments
though.

> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    /* Usually device_del will remove related chardev as well,
> +     * So we don't need to check its return value.
> +     */

How about:

DeviceDel from Detach may remove chardev, so we cannot rely on return
status to delete TLS chardevs.

> +    ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
> +
> +    if (tlsAlias)
> +        ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias));
> +    if (secAlias)
> +        ignore_value(qemuMonitorDelObject(priv->mon, secAlias));

These would go in the helper...

> +
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;

And right here it's (conditions from above):

    if (redirdev->source->type == VIR_DOMAIN_CHR_TYPE_TCP &&
        redirdev->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES &&
        qemuDomainDelChardevTLSObjects(driver, vm, charAlias) < 0)
        goto cleanup;


> +
> +    virDomainAuditRedirdev(vm, redirdev, "detach", true);
> +
> +    event = virDomainEventDeviceRemovedNewFromObj(vm, redirdev->info.alias);
> +    qemuDomainEventQueue(driver, event);
> +
> +    if ((idx = virDomainRedirdevDefFind(vm->def, redirdev)) >= 0)
> +        virDomainRedirdevDefRemove(vm->def, idx);
> +    qemuDomainReleaseDeviceAddress(vm, &redirdev->info, NULL);
> +    virDomainRedirdevDefFree(redirdev);
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(charAlias);
> +    VIR_FREE(tlsAlias);
> +    VIR_FREE(secAlias);

    virObjectUnref(cfg);

(of course w/ common function it wouldn't be necessary)

> +    return ret;
> +}
> +
>  
>  int
>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
> @@ -5112,6 +5180,49 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>      return ret;
>  }
>  

Two empty lines

> +int
> +qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
> +                                   virDomainObjPtr vm,
> +                                   virDomainRedirdevDefPtr redirdev)

Indent is too much for 2nd/3rd args...

follow the Shmem code and call this "dev" (or "def" like Input)... IOW:
Something short.

> +{
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainDefPtr vmdef = vm->def;

No need for vmdef - it's just used once

> +    virDomainRedirdevDefPtr tmpRedirdevDef;

and this then becomes redirdev since in reality it *is* the one to be
found in the domain.

> +    ssize_t idx;
> +
> +    if ((idx = virDomainRedirdevDefFind(vmdef, redirdev)) < 0) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("no matching redirdev was not found"));
> +        return -1;
> +    }
> +
> +    tmpRedirdevDef = vm->def->redirdevs[idx];

You "could" have used vmdef here, but I'll just remove it and access
directly.

> +
> +    if (!tmpRedirdevDef->info.alias) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("alias not set for redirdev device"));
> +        return -1;
> +    }
> +
> +    qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdevDef->info);
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    if (qemuMonitorDelDevice(priv->mon, tmpRedirdevDef->info.alias) < 0) {
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        goto cleanup;
> +    }
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;
> +
> +    if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
> +        ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdevDef);
> +
> + cleanup:
> +    qemuDomainResetDeviceRemoval(vm);
> +    return ret;
> +}
> +
>  
>  int
>  qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 3e0d618e0..6c642c4fd 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -125,6 +125,9 @@ int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>  int qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>                               virDomainObjPtr vm,
>                               virDomainWatchdogDefPtr watchdog);

Blank line for readability

> +int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver,
> +                                   virDomainObjPtr vm,
> +                                   virDomainRedirdevDefPtr redirdev);

Use @dev (or @def) here for consistency.

>  
>  int qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
>                                  virDomainObjPtr vm,
> 

John