[libvirt] [PATCH 11/12] qemu: Implement RNG device hotplug on live level

[lhuang]

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 at 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
>
>