[libvirt] [PATCHv2 3/4] qemu: Implement share memory device hot-plug

[lhuang]

On 12/10/2015 09:23 AM, John Ferlan wrote:
> $SUBJ
>
> s/share/shared

thanks for pointing out this

>
> On 11/26/2015 04:06 AM, Luyao Huang wrote:
>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>> ---
>>   src/qemu/qemu_driver.c  | 10 ++++++++-
>>   src/qemu/qemu_hotplug.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/qemu/qemu_hotplug.h |  3 +++
>>   3 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 5ded9ef..3c25c07 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -7784,6 +7784,15 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
>>           dev->data.memory = NULL;
>>           break;
>>   
>> +    case VIR_DOMAIN_DEVICE_SHMEM:
>> +        ret = qemuDomainAttachShmemDevice(driver, vm,
>> +                                          dev->data.shmem);
>> +        if (!ret) {
>> +            alias = dev->data.shmem->info.alias;
>> +            dev->data.shmem = NULL;
>> +        }
>> +        break;
>> +
>>       case VIR_DOMAIN_DEVICE_NONE:
>>       case VIR_DOMAIN_DEVICE_FS:
>>       case VIR_DOMAIN_DEVICE_INPUT:
>> @@ -7795,7 +7804,6 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
>>       case VIR_DOMAIN_DEVICE_SMARTCARD:
>>       case VIR_DOMAIN_DEVICE_MEMBALLOON:
>>       case VIR_DOMAIN_DEVICE_NVRAM:
>> -    case VIR_DOMAIN_DEVICE_SHMEM:
>>       case VIR_DOMAIN_DEVICE_TPM:
>>       case VIR_DOMAIN_DEVICE_PANIC:
>>       case VIR_DOMAIN_DEVICE_LAST:
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 8804d3d..c5e544d 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -1882,6 +1882,64 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>>   }
>>   
>>   
> It seems this is modeled after qemuDomainAttachRNGDevice, right?

Right :)

>> +int
>> +qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
>> +                            virDomainObjPtr vm,
>> +                            virDomainShmemDefPtr shmem)
>> +{
>> +    int ret = -1;
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    char *devstr = NULL;
>> +    char *charAlias = NULL;
>> +
>> +    if (virAsprintf(&shmem->info.alias, "shmem%zu", vm->def->nshmems) < 0)
>> +        return -1;
>> +
>> +    if (VIR_REALLOC_N(vm->def->shmems, vm->def->nshmems + 1) < 0)
>> +        return -1;
>> +
>> +    if ((shmem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
>> +         shmem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
>> +         (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &shmem->info) < 0))
>> +        return -1;
>> +
>> +    if (!(devstr = qemuBuildShmemDevStr(vm->def, shmem, priv->qemuCaps)))
>> +        goto cleanup;
>> +
>
>> +    if (virAsprintf(&charAlias, "char%s", shmem->info.alias) < 0)
>> +        goto cleanup;
>> +
> This seems to be up to a 2 stage process "if" server.enabled" is true"....

Indeed, i will fix it in next version

>
>> +    qemuDomainObjEnterMonitor(driver, vm);
>> +
>> +    if (shmem->server.enabled &&
>> +        qemuMonitorAttachCharDev(priv->mon, charAlias,
>> +                                 &shmem->server.chr) < 0) {
> Instead of the following change to:
>
>             goto failchardev;
>
> and the {} won't be necessary
>
...
> Instead of the following change to
>
>             goto failadddevice;
>
> and the {} won't be necessary
...
> Again following RNG model - this should have a
>
>      if (*Exit*() < 0) {
>          vm = NULL;
>          goto cleanup
>      }
...
> And of course the auditing change as well.
...
> Following RNG the && vm would be used here... See your patch commits 
> '0ed3b3353' and '980b265d0'
...
>   failadddevice:
>      if (shmem->server.enabled)
>          ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias));
>
>   failchardev:
>      ignore_value(qemuDomainObjExitMonitor(driver, vm));
>      goto cleanup;
>

Nice improve, i must forgot a old problem that the &vm will be changed 
after qemu unexpected exit during enter the monitor.

> Hope this all makes sense (it's been a long day ;-))

Thanks a lot for your review and suggestion, and i am sorry that i 
didn't reply your mails when i saw them (I'm kind of overwhelmed lately :) )

Have a nice day !

Luyao

>
> John
>> +}
>> +
>> +
>>   static int
>>   qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
>>                                 virDomainObjPtr vm,
>> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
>> index 4140da3..60137a6 100644
>> --- a/src/qemu/qemu_hotplug.h
>> +++ b/src/qemu/qemu_hotplug.h
>> @@ -109,6 +109,9 @@ int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
>>   int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver,
>>                                 virDomainObjPtr vm,
>>                                 virDomainRNGDefPtr rng);
>> +int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
>> +                                virDomainObjPtr vm,
>> +                                virDomainShmemDefPtr shmem);
>>   
>>   int
>>   qemuDomainChrInsert(virDomainDefPtr vmdef,
>>