12:15 a.m.
On 01/05/2015 11:48 PM, Peter Krempa wrote:
On 01/03/15 06:06, Luyao Huang wrote:
> We have enough patches for hotplug RNG device, maybe we can
> implement live hotplug of a RNG device.
>
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 8 ++++-
> src/qemu/qemu_hotplug.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_hotplug.h | 3 ++
> 3 files changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index f9327b4..7f1e612 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1523,6 +1523,98 @@ qemuDomainRNGRemove(virDomainDefPtr vmdef,
> return ret;
> }
>
> +int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainRNGDefPtr rng)
> +{
> + int ret = -1;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + virDomainDefPtr vmdef = vm->def;
> + char *devstr = NULL;
> + char *charAlias = NULL;
> + char *objAlias = NULL;
> + bool need_remove = false;
> + bool releaseaddr = false;
> +
> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("qemu does not support -device"));
> + return ret;
> + }
Additionally to the -device support, qemu also needs to support chardev
hotplug and the rng device with the appropriate backends, we already
have capability bits for them so you need to check them here.
Thanks for your
review.
Yes, i forgot these check in this place and i found
QEMU_CAPS_OBJECT_RNG_RANDOM
and QEMU_CAPS_OBJECT_RNG_EGD for random and egd backend RNG device.
But i cannot found a capability bit for "support chardev hotplug", maybe
this one? QEMU_CAPS_CHARDEV
> +
> + if (qemuAssignDeviceRNGAlias(vmdef, rng, -1) < 0)
> + return ret;
> +
> + if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> + if (STRPREFIX(vm->def->os.machine, "s390-ccw") &&
> + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
> + rng->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
> + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
> + rng->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
> + }
> + }
> +
> + if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
> + rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> + if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &rng->info) <
0)
> + return ret;
> + } else if (rng->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> + if (virDomainCCWAddressAssign(&rng->info, priv->ccwaddrs,
> + !rng->info.addr.ccw.assigned) < 0)
Line above is misaligned.
Thanks
> + return ret;
> + }
> + releaseaddr = true;
> + if (!(devstr = qemuBuildRNGDevStr(vmdef, rng, priv->qemuCaps)))
> + return ret;
After you set releaseaddr to true you need to jump to cleanup in case of
error so that the address gets cleared.
Yes, i have made a mistake here. I will fix
it in next version.
> +
> + if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0)
> + goto cleanup;
> +
> + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) {
> + if (virAsprintf(&charAlias, "char%s", rng->info.alias) <
0)
> + goto cleanup;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> + if (qemuMonitorAttachCharDev(priv->mon, charAlias,
rng->source.chardev) < 0) {
> + qemuDomainObjExitMonitor(driver, vm);
> + goto audit;
> + }
> + need_remove = true;
This variable should be named "remove_chardev" so that it's clear what
it's used to.
Okay, thanks
> + } else {
> + qemuDomainObjEnterMonitor(driver, vm);
> + }
> +
> + if (qemuMonitorAttachRNGDev(priv->mon, charAlias, objAlias, rng) < 0) {
> + if (need_remove)
> + qemuMonitorDetachCharDev(priv->mon, charAlias);
> + qemuDomainObjExitMonitor(driver, vm);
> + goto audit;
> + }
> +
> + if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) {
> + qemuMonitorDetachRNGDev(priv->mon, objAlias);
> + if (need_remove)
> + qemuMonitorDetachCharDev(priv->mon, charAlias);
> + qemuDomainObjExitMonitor(driver, vm);
> + goto audit;
> + }
> + qemuDomainObjExitMonitor(driver, vm);
> +
> + if (qemuDomainRNGInsert(vmdef, rng) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + audit:
> + virDomainAuditRNG(vm, NULL, rng, "attach", ret == 0);
> + cleanup:
> + if (releaseaddr)
> + qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL);
> + VIR_FREE(charAlias);
> + VIR_FREE(objAlias);
> + VIR_FREE(devstr);
> + return ret;
> +}
> +
>
> static int
> qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
The rest looks good.
Peter