Re: [libvirt] [PATCHv2 2/4] qemu: Implement shared memory device cold (un)plug

On 11/26/2015 04:06 AM, Luyao Huang wrote: > Add support for using the attach/detach device APIs on the inactive > configuration to add/del shared memory devices. > > Signed-off-by: Luyao Huang <lhuang(a)redhat.com&gt; > --- > src/libvirt_private.syms | 1 + > src/qemu/qemu_driver.c | 21 +++++++++++++++++++-- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 88c2c53..c89d27a 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -451,6 +451,7 @@ virDomainSaveStatus; > virDomainSaveXML; > virDomainSeclabelTypeFromString; > virDomainSeclabelTypeToString; > +virDomainShmemDefFree; > virDomainShmemFind; > virDomainShmemInsert; > virDomainShmemRemove; > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 65ccf99..5ded9ef 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -8174,6 +8174,15 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, > dev->data.memory = NULL; > break; > > + case VIR_DOMAIN_DEVICE_SHMEM: > + if (virDomainShmemInsert(vmdef, dev->data.shmem) < 0) > + return -1; > + dev->data.shmem = NULL; > + > + if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) > + return -1; > + break; > + > case VIR_DOMAIN_DEVICE_INPUT: > case VIR_DOMAIN_DEVICE_SOUND: > case VIR_DOMAIN_DEVICE_VIDEO: > @@ -8183,7 +8192,6 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, > case VIR_DOMAIN_DEVICE_SMARTCARD: > case VIR_DOMAIN_DEVICE_MEMBALLOON: > case VIR_DOMAIN_DEVICE_NVRAM: > - case VIR_DOMAIN_DEVICE_SHMEM: > case VIR_DOMAIN_DEVICE_REDIRDEV: > case VIR_DOMAIN_DEVICE_NONE: > case VIR_DOMAIN_DEVICE_TPM: > @@ -8310,6 +8318,16 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, > virDomainMemoryDefFree(virDomainMemoryRemove(vmdef, idx)); > break; > > + case VIR_DOMAIN_DEVICE_SHMEM: > + if ((idx = virDomainShmemFind(vmdef, dev->data.shmem, true)) < 0) { Here rather than 'true' which to me only has meaning if I go read the code... Of course same thing holds true when passing a 0 (zero) flags value. BTW: My idea around flags matches what I recently added for VolReadErrorMode in storage_backend.h... But for this it'd be something like the following in (I assume) domain_conf.h near _virDomainShmemDef. /* DomainShmemMatchFlags * Flags to dictate the level of matching when searching for a * shmem object in the domain 'nshmems' list. */ enum { VIR_DOMAIN_SHMEM_MATCH_NAME_ONLY = 1 << 0, } In general though it seems fine.

John > + virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + _("no matching shared memory device was found")); > + return -1; > + } > + > + virDomainShmemDefFree(virDomainShmemRemove(vmdef, idx)); > + break; > + > case VIR_DOMAIN_DEVICE_INPUT: > case VIR_DOMAIN_DEVICE_SOUND: > case VIR_DOMAIN_DEVICE_VIDEO: > @@ -8319,7 +8337,6 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, > case VIR_DOMAIN_DEVICE_SMARTCARD: > case VIR_DOMAIN_DEVICE_MEMBALLOON: > case VIR_DOMAIN_DEVICE_NVRAM: > - case VIR_DOMAIN_DEVICE_SHMEM: > case VIR_DOMAIN_DEVICE_REDIRDEV: > case VIR_DOMAIN_DEVICE_NONE: > case VIR_DOMAIN_DEVICE_TPM: >