[libvirt] [PATCH v2 8/8] qemu: Implement chardev hotplug on live level

[Martin Kletzander]

On 06/06/2013 02:30 PM, Michal Privoznik wrote:
> Since previous patches has prepared everything for us, we may now

s/has/have/

> implement live hotplug of a character device.
> ---
>  src/qemu/qemu_driver.c  |  10 +++++
>  src/qemu/qemu_hotplug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_hotplug.h |   6 +++
>  3 files changed, 118 insertions(+)
> 
[...]
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 6c07af5..3e3bf2f 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1192,6 +1192,63 @@ error:
>  
>  }
>  
> +int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
> +                              virDomainObjPtr vm,
> +                              virDomainChrDefPtr chr)
> +{
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainDefPtr vmdef = vm->def;
> +    char *devstr = NULL;
> +    char *charAlias = NULL;
> +
> +    if (virDomainChrFind(vmdef, chr)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("chardev already exists"));
> +        return ret;
> +    }
> +
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("qemu does not support -device"));
> +        return ret;
> +    }
> +
> +    if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0)
> +        return ret;
> +
> +    if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0)
> +        return ret;
> +
> +    if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) {
> +        qemuDomainObjExitMonitor(driver, vm);
> +        goto cleanup;
> +    }
> +
> +    if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) {
> +        /* detach associated chardev on error */
> +        qemuMonitorDetachCharDev(priv->mon, charAlias);
> +        qemuDomainObjExitMonitor(driver, vm);
> +        goto cleanup;
> +    }
> +    qemuDomainObjExitMonitor(driver, vm);
> +
> +    if (virDomainChrInsert(vmdef, chr) < 0)
> +        goto cleanup;

This shouldn't fail, but (and similarly to 'Insert'), do we want to keep
the device to be kept there?  To avoid those problems, you could move
this code before the attaching the device in and just remove it from the
DefPtr in case the attach isn't successful.  You should do it similarly
for 'Remove', but there's no place to fail, or is there?

Also, you should use virDomainAuditChardevPath() when adding/removing
the backend.

ACK with those points addressed.

Could you add a test for the hotplug as well?

Martin