[libvirt] [PATCHv2 0/4] Implement shared memory device (cold) hot-plug/unplug
v1: https://www.redhat.com/archives/libvir-list/2015-June/msg00830.html These patches implement the hot-plug/hot-unplug and cold-plug/cold-unplug for Inter-VM Shared Memory PCI device. Luyao Huang (4): conf: Add helpers to insert/remove/find shmem devices in domain def qemu: Implement shared memory device cold (un)plug qemu: Implement share memory device hot-plug qemu: Implement shared memory device hot-unplug src/conf/domain_conf.c | 66 ++++++++++++++++++++ src/conf/domain_conf.h | 7 +++ src/libvirt_private.syms | 4 ++ src/qemu/qemu_driver.c | 35 +++++++++-- src/qemu/qemu_hotplug.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 6 ++ 6 files changed, 265 insertions(+), 5 deletions(-) -- 1.8.3.1
The helpers will be useful when implementing hotplug and coldplug of shared memory devices. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 7 +++++ src/libvirt_private.syms | 3 +++ 3 files changed, 76 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 616bf80..cc2a767 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13950,6 +13950,72 @@ virDomainMemoryRemove(virDomainDefPtr def, } +int +virDomainShmemInsert(virDomainDefPtr def, + virDomainShmemDefPtr shmem) +{ + return VIR_APPEND_ELEMENT(def->shmems, def->nshmems, shmem); +} + + +ssize_t +virDomainShmemFind(virDomainDefPtr def, + virDomainShmemDefPtr shmem, + bool fullmatch) +{ + size_t i; + + for (i = 0; i < def->nshmems; i++) { + virDomainShmemDefPtr tmpshmem = def->shmems[i]; + + if (STREQ(shmem->name, tmpshmem->name)) { + if (!fullmatch) + return i; + } else { + continue; + } + + if (shmem->size != tmpshmem->size) + continue; + + if (shmem->server.enabled != tmpshmem->server.enabled || + (shmem->server.enabled && + STRNEQ_NULLABLE(shmem->server.chr.data.nix.path, + tmpshmem->server.chr.data.nix.path))) + continue; + + if (shmem->msi.enabled != tmpshmem->msi.enabled || + (shmem->msi.enabled && + (shmem->msi.vectors != tmpshmem->msi.vectors || + shmem->msi.ioeventfd != tmpshmem->msi.ioeventfd))) + continue; + + if (shmem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&shmem->info, &tmpshmem->info)) + continue; + + break; + } + + if (i < def->nshmems) + return i; + + return -1; +} + + +virDomainShmemDefPtr +virDomainShmemRemove(virDomainDefPtr def, + size_t idx) +{ + virDomainShmemDefPtr ret = def->shmems[idx]; + + VIR_DELETE_ELEMENT(def->shmems, idx, def->nshmems); + + return ret; +} + + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8d43ee6..ee76e3f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3012,6 +3012,13 @@ int virDomainMemoryFindInactiveByDef(virDomainDefPtr def, virDomainMemoryDefPtr mem) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virDomainShmemInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +ssize_t virDomainShmemFind(virDomainDefPtr def, virDomainShmemDefPtr shmem, bool fullmatch) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +virDomainShmemDefPtr virDomainShmemRemove(virDomainDefPtr def, size_t idx) + ATTRIBUTE_NONNULL(1); + VIR_ENUM_DECL(virDomainTaint) VIR_ENUM_DECL(virDomainVirt) VIR_ENUM_DECL(virDomainBoot) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7e60d87..88c2c53 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -451,6 +451,9 @@ virDomainSaveStatus; virDomainSaveXML; virDomainSeclabelTypeFromString; virDomainSeclabelTypeToString; +virDomainShmemFind; +virDomainShmemInsert; +virDomainShmemRemove; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString; -- 1.8.3.1
On 11/26/2015 04:06 AM, Luyao Huang wrote:
The helpers will be useful when implementing hotplug and coldplug of shared memory devices.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 7 +++++ src/libvirt_private.syms | 3 +++ 3 files changed, 76 insertions(+)
Although there is a v1, it doesn't seem patches 6-10 of that series (e.g. these patches) were reviewed. Is that correct? Just want to make sure I'm not missing something from someone else's review... anyway...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 616bf80..cc2a767 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13950,6 +13950,72 @@ virDomainMemoryRemove(virDomainDefPtr def, }
+int +virDomainShmemInsert(virDomainDefPtr def, + virDomainShmemDefPtr shmem)
I see you've followed the virDomainHostdevInsert model...
+{ + return VIR_APPEND_ELEMENT(def->shmems, def->nshmems, shmem); +} + +
Would be nice to have an intro comment explaining the params (for other two as well), but this one is a bit more "interesting" because of arg3. This includes adding return value.
+ssize_t +virDomainShmemFind(virDomainDefPtr def,
More or less follows virDomainHostdevFind, but could also be more explicit using "FindIndex" or "FindIdx"... Not that important.
+ virDomainShmemDefPtr shmem, + bool fullmatch)
I think this is more "match name only". It could also be an "unsigned int flags" where the flags is an enum that could dictate the level of match required by the caller (perhaps overkill for what is necessary, but flags just seems to be a better option in my opinion. A passed value of 0 for flags would indicate to match everything Beyond that, following virDomainHostdevFind - you could perhaps return an int. We know size_t will be 0 to def->nshmems...
+{ + size_t i; + + for (i = 0; i < def->nshmems; i++) { + virDomainShmemDefPtr tmpshmem = def->shmems[i]; + + if (STREQ(shmem->name, tmpshmem->name)) { + if (!fullmatch) + return i; + } else { + continue; + }
Perhaps cleaner as: if (STRNEQ(shmem->name, tmpshmem->name)) continue; if (!fullmatch) return i; You could also use matchnameonly or (flags & VIR_DOMAIN_SHMEM_NAME_MATCH_ONLY)
+ + if (shmem->size != tmpshmem->size) + continue; +
Eventually someone could add other flags such as -> match name and size only... -> match name, size, and server -> match name, size, server, and msi -> match name, size, server, msi, and info/address
+ if (shmem->server.enabled != tmpshmem->server.enabled || + (shmem->server.enabled && + STRNEQ_NULLABLE(shmem->server.chr.data.nix.path, + tmpshmem->server.chr.data.nix.path))) + continue; + + if (shmem->msi.enabled != tmpshmem->msi.enabled || + (shmem->msi.enabled && + (shmem->msi.vectors != tmpshmem->msi.vectors || + shmem->msi.ioeventfd != tmpshmem->msi.ioeventfd))) + continue; + + if (shmem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&shmem->info, &tmpshmem->info)) + continue;
Matching address is such a touchy thing, I know why it's here to be complete, but since the remove device doesn't have to add it to the XML passed in, is there a need? I guess we can leave it for now unless someone else offers up a different opinion.
+ + break;
Why not just return i; here?
+ } + + if (i < def->nshmems) + return i;
Removing need for this check
+ + return -1; +} + + +virDomainShmemDefPtr +virDomainShmemRemove(virDomainDefPtr def, + size_t idx) +{ + virDomainShmemDefPtr ret = def->shmems[idx]; + + VIR_DELETE_ELEMENT(def->shmems, idx, def->nshmems); + + return ret; +} + + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8d43ee6..ee76e3f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3012,6 +3012,13 @@ int virDomainMemoryFindInactiveByDef(virDomainDefPtr def, virDomainMemoryDefPtr mem) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+int virDomainShmemInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +ssize_t virDomainShmemFind(virDomainDefPtr def, virDomainShmemDefPtr shmem, bool fullmatch) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +virDomainShmemDefPtr virDomainShmemRemove(virDomainDefPtr def, size_t idx) + ATTRIBUTE_NONNULL(1); + VIR_ENUM_DECL(virDomainTaint) VIR_ENUM_DECL(virDomainVirt) VIR_ENUM_DECL(virDomainBoot) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7e60d87..88c2c53 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -451,6 +451,9 @@ virDomainSaveStatus; virDomainSaveXML; virDomainSeclabelTypeFromString; virDomainSeclabelTypeToString; +virDomainShmemFind; +virDomainShmemInsert; +virDomainShmemRemove; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString;
On 12/10/2015 08:50 AM, John Ferlan wrote:
On 11/26/2015 04:06 AM, Luyao Huang wrote:
The helpers will be useful when implementing hotplug and coldplug of shared memory devices.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 7 +++++ src/libvirt_private.syms | 3 +++ 3 files changed, 76 insertions(+)
Although there is a v1, it doesn't seem patches 6-10 of that series (e.g. these patches) were reviewed. Is that correct? Just want to make sure I'm not missing something from someone else's review...
anyway...
here is the review of patch 10: https://www.redhat.com/archives/libvir-list/2015-July/msg00338.html patch 6: https://www.redhat.com/archives/libvir-list/2015-July/msg00319.html And patch 7-9 were not reviewed, i posted a new version since i introduced a new parameter in virDomainShmemFind() and removed audit part (I remember Martin will help finish that part, but i am sorry that i forgot mention this in some place)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 616bf80..cc2a767 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13950,6 +13950,72 @@ virDomainMemoryRemove(virDomainDefPtr def, }
+int +virDomainShmemInsert(virDomainDefPtr def, + virDomainShmemDefPtr shmem) I see you've followed the virDomainHostdevInsert model...
+{ + return VIR_APPEND_ELEMENT(def->shmems, def->nshmems, shmem); +} + + Would be nice to have an intro comment explaining the params (for other two as well), but this one is a bit more "interesting" because of arg3. This includes adding return value.
Okay, i will add them in next version.
+ssize_t +virDomainShmemFind(virDomainDefPtr def, More or less follows virDomainHostdevFind, but could also be more explicit using "FindIndex" or "FindIdx"... Not that important.
+ virDomainShmemDefPtr shmem, + bool fullmatch) I think this is more "match name only". It could also be an "unsigned int flags" where the flags is an enum that could dictate the level of match required by the caller (perhaps overkill for what is necessary, but flags just seems to be a better option in my opinion. A passed value of 0 for flags would indicate to match everything
interesting idea, i will have a try with your option, you will see it in next version :)
Beyond that, following virDomainHostdevFind - you could perhaps return an int. We know size_t will be 0 to def->nshmems...
got it.
+{ + size_t i; + + for (i = 0; i < def->nshmems; i++) { + virDomainShmemDefPtr tmpshmem = def->shmems[i]; + + if (STREQ(shmem->name, tmpshmem->name)) { + if (!fullmatch) + return i; + } else { + continue; + } Perhaps cleaner as:
if (STRNEQ(shmem->name, tmpshmem->name)) continue;
if (!fullmatch) return i;
You could also use matchnameonly or (flags & VIR_DOMAIN_SHMEM_NAME_MATCH_ONLY)
Okay
+ + if (shmem->size != tmpshmem->size) + continue; + Eventually someone could add other flags such as
-> match name and size only... -> match name, size, and server -> match name, size, server, and msi -> match name, size, server, msi, and info/address
I see.
+ if (shmem->server.enabled != tmpshmem->server.enabled || + (shmem->server.enabled && + STRNEQ_NULLABLE(shmem->server.chr.data.nix.path, + tmpshmem->server.chr.data.nix.path))) + continue; + + if (shmem->msi.enabled != tmpshmem->msi.enabled || + (shmem->msi.enabled && + (shmem->msi.vectors != tmpshmem->msi.vectors || + shmem->msi.ioeventfd != tmpshmem->msi.ioeventfd))) + continue; + + if (shmem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&shmem->info, &tmpshmem->info)) + continue; Matching address is such a touchy thing, I know why it's here to be complete, but since the remove device doesn't have to add it to the XML passed in, is there a need? I guess we can leave it for now unless someone else offers up a different opinion.
Indeed, i will remove this check in next version.
+ + break; Why not just return i; here?
You are right, i will fix this and ...
+ } + + if (i < def->nshmems) + return i; Removing need for this check
... this in next version Thanks a lot for your reviews and suggestions. Luyao
+ + return -1; +} + + +virDomainShmemDefPtr +virDomainShmemRemove(virDomainDefPtr def, + size_t idx) +{ + virDomainShmemDefPtr ret = def->shmems[idx]; + + VIR_DELETE_ELEMENT(def->shmems, idx, def->nshmems); + + return ret; +} + + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8d43ee6..ee76e3f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3012,6 +3012,13 @@ int virDomainMemoryFindInactiveByDef(virDomainDefPtr def, virDomainMemoryDefPtr mem) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+int virDomainShmemInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +ssize_t virDomainShmemFind(virDomainDefPtr def, virDomainShmemDefPtr shmem, bool fullmatch) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +virDomainShmemDefPtr virDomainShmemRemove(virDomainDefPtr def, size_t idx) + ATTRIBUTE_NONNULL(1); + VIR_ENUM_DECL(virDomainTaint) VIR_ENUM_DECL(virDomainVirt) VIR_ENUM_DECL(virDomainBoot) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7e60d87..88c2c53 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -451,6 +451,9 @@ virDomainSaveStatus; virDomainSaveXML; virDomainSeclabelTypeFromString; virDomainSeclabelTypeToString; +virDomainShmemFind; +virDomainShmemInsert; +virDomainShmemRemove; virDomainShutdownReasonTypeFromString; virDomainShutdownReasonTypeToString; virDomainShutoffReasonTypeFromString;
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@redhat.com> --- 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) { + 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: -- 1.8.3.1
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@redhat.com> --- 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:
On 12/10/2015 08:53 AM, John Ferlan wrote:
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@redhat.com> --- 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.
Okay, i have saw your comment mentioned this in another mail, and i will post a new version include your idea. Thanks a lot for your review and suggestion. Luyao
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:
Signed-off-by: Luyao Huang <lhuang@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, } +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; + + qemuDomainObjEnterMonitor(driver, vm); + + if (shmem->server.enabled && + qemuMonitorAttachCharDev(priv->mon, charAlias, + &shmem->server.chr) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { + if (shmem->server.enabled) + ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (virDomainShmemInsert(vm->def, shmem) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (ret < 0) + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + VIR_FREE(charAlias); + VIR_FREE(devstr); + return ret; +} + + 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, -- 1.8.3.1
$SUBJ s/share/shared On 11/26/2015 04:06 AM, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@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?
+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"...
+ 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
+ ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
Instead of the following change to goto failadddevice; and the {} won't be necessary
+ if (shmem->server.enabled) + ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;
Again following RNG model - this should have a if (*Exit*() < 0) { vm = NULL; goto cleanup }
+ + if (virDomainShmemInsert(vm->def, shmem) < 0) + goto cleanup;
And of course the auditing change as well.
+ + ret = 0; + cleanup: + if (ret < 0)
Following RNG the && vm would be used here... See your patch commits '0ed3b3353' and '980b265d0'
+ qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + VIR_FREE(charAlias); + VIR_FREE(devstr); + return ret;
failadddevice: if (shmem->server.enabled) ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); failchardev: ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto cleanup; Hope this all makes sense (it's been a long day ;-)) 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,
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@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,
[...]
+int +qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + char *devstr = NULL; + char *charAlias = NULL;
Dang... one more thing... why no? if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("qemu does not support -device")); return -1; } I see it's not in AttachRNG (so why it was missed), *BUT* it is in DetachRNG... So it seems a separate patch for AttachRNG should add that? John
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 4 ++- src/qemu/qemu_hotplug.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ 3 files changed, 99 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3c25c07..9e7429a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7881,6 +7881,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_MEMORY: ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory); break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem); + break; case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -7892,7 +7895,6 @@ qemuDomainDetachDeviceLive(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_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c5e544d..4ab05f3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3347,6 +3347,45 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, } +static int +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + virObjectEventPtr event; + char *charAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + ssize_t idx; + int rc = 0; + + VIR_DEBUG("Removing shared memory device %s from domain %p %s", + shmem->info.alias, vm, vm->def->name); + + if (shmem->server.enabled) { + if (virAsprintf(&charAlias, "char%s", shmem->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + VIR_FREE(charAlias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + } + + if (rc < 0) + return -1; + + if ((event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias))) + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainShmemFind(vm->def, shmem, true)) >= 0) + virDomainShmemRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + virDomainShmemDefFree(shmem); + return 0; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3378,6 +3417,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory); break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem); + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: @@ -3391,7 +3434,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, 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: @@ -4378,3 +4420,53 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, qemuDomainResetDeviceRemoval(vm); return ret; } + + +int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + ssize_t idx; + virDomainShmemDefPtr tmpshmem; + int rc; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return -1; + } + + if ((idx = virDomainShmemFind(vm->def, shmem, true)) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + tmpshmem = vm->def->shmems[idx]; + + if (!tmpshmem->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("alias not set for shared memory device")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmpshmem->info); + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDelDevice(priv->mon, tmpshmem->info.alias); + if (qemuDomainObjExitMonitor(driver, vm) || rc < 0) + goto cleanup; + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveShmemDevice(driver, vm, tmpshmem); + else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 60137a6..1b18e8a 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -112,6 +112,9 @@ int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainShmemDefPtr shmem); +int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem); int qemuDomainChrInsert(virDomainDefPtr vmdef, -- 1.8.3.1
On 11/26/2015 04:07 AM, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 4 ++- src/qemu/qemu_hotplug.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ 3 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3c25c07..9e7429a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7881,6 +7881,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_MEMORY: ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory); break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem); + break;
case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -7892,7 +7895,6 @@ qemuDomainDetachDeviceLive(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_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c5e544d..4ab05f3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3347,6 +3347,45 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, }
+static int +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + virObjectEventPtr event; + char *charAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + ssize_t idx; + int rc = 0; + + VIR_DEBUG("Removing shared memory device %s from domain %p %s", + shmem->info.alias, vm, vm->def->name); + + if (shmem->server.enabled) { + if (virAsprintf(&charAlias, "char%s", shmem->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + VIR_FREE(charAlias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + } +
Auditing code?
+ if (rc < 0) + return -1; +
I know this is a copy of the RemoveRNGDevice; however, this code doesn't remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter the monitor at all. Thus the following event probably won't happen...
+ if ((event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias))) + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainShmemFind(vm->def, shmem, true)) >= 0) + virDomainShmemRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + virDomainShmemDefFree(shmem); + return 0; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3378,6 +3417,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory); break;
+ case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem); + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: @@ -3391,7 +3434,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, 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: @@ -4378,3 +4420,53 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, qemuDomainResetDeviceRemoval(vm); return ret; } + + +int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + ssize_t idx; + virDomainShmemDefPtr tmpshmem; + int rc; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return -1; + }
Well it's here, but not in AttachShmemDevice...
+ + if ((idx = virDomainShmemFind(vm->def, shmem, true)) < 0) {
Again we could use a 'flags' argument instead... Of course there isn't a "false" argument to any of the added callers to date, so not sure of the need for it... Could still use/add flags and keep it as "unused" for now.
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration"));
s/(("/(("shared memory / John
+ return -1; + } + + tmpshmem = vm->def->shmems[idx]; + + if (!tmpshmem->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("alias not set for shared memory device")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmpshmem->info); + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDelDevice(priv->mon, tmpshmem->info.alias); + if (qemuDomainObjExitMonitor(driver, vm) || rc < 0) + goto cleanup; + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveShmemDevice(driver, vm, tmpshmem); + else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 60137a6..1b18e8a 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -112,6 +112,9 @@ int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainShmemDefPtr shmem); +int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem);
int qemuDomainChrInsert(virDomainDefPtr vmdef,
On 12/10/2015 09:43 AM, John Ferlan wrote:
On 11/26/2015 04:07 AM, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 4 ++- src/qemu/qemu_hotplug.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ 3 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3c25c07..9e7429a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7881,6 +7881,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_MEMORY: ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory); break; + case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem); + break;
case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -7892,7 +7895,6 @@ qemuDomainDetachDeviceLive(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_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c5e544d..4ab05f3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3347,6 +3347,45 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, }
+static int +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + virObjectEventPtr event; + char *charAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + ssize_t idx; + int rc = 0; + + VIR_DEBUG("Removing shared memory device %s from domain %p %s", + shmem->info.alias, vm, vm->def->name); + + if (shmem->server.enabled) { + if (virAsprintf(&charAlias, "char%s", shmem->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + VIR_FREE(charAlias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + } + Auditing code?
I have removed them since i remember @Martin will finish the audit part.
+ if (rc < 0) + return -1; + I know this is a copy of the RemoveRNGDevice; however, this code doesn't remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter the monitor at all.
Thus the following event probably won't happen...
I am not sure what your mean is ... i guess your mean the device remove event we get from qmp monitor won't happen ? we will get that event if qemu remove shmem device success, it should always happen if qemu really remove it and there is no bugs on qemu :)
+ if ((event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias))) + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainShmemFind(vm->def, shmem, true)) >= 0) + virDomainShmemRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); + virDomainShmemDefFree(shmem); + return 0; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3378,6 +3417,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory); break;
+ case VIR_DOMAIN_DEVICE_SHMEM: + ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem); + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: @@ -3391,7 +3434,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, 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: @@ -4378,3 +4420,53 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, qemuDomainResetDeviceRemoval(vm); return ret; } + + +int +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + ssize_t idx; + virDomainShmemDefPtr tmpshmem; + int rc; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return -1; + } Well it's here, but not in AttachShmemDevice...
I must forgot add it in the AttachShmemDevice, thanks for pointing out.
+ + if ((idx = virDomainShmemFind(vm->def, shmem, true)) < 0) { Again we could use a 'flags' argument instead... Of course there isn't a "false" argument to any of the added callers to date, so not sure of the need for it... Could still use/add flags and keep it as "unused" for now.
Okay
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); s/(("/(("shared memory /
I will fix it in next version, Thanks a lot for your review Luyao
John
+ return -1; + } + + tmpshmem = vm->def->shmems[idx]; + + if (!tmpshmem->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("alias not set for shared memory device")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmpshmem->info); + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDelDevice(priv->mon, tmpshmem->info.alias); + if (qemuDomainObjExitMonitor(driver, vm) || rc < 0) + goto cleanup; + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveShmemDevice(driver, vm, tmpshmem); + else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 60137a6..1b18e8a 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -112,6 +112,9 @@ int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainShmemDefPtr shmem); +int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr shmem);
int qemuDomainChrInsert(virDomainDefPtr vmdef,
[...]
+ if (rc < 0) + return -1; + I know this is a copy of the RemoveRNGDevice; however, this code doesn't remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter the monitor at all.
Thus the following event probably won't happen...
I am not sure what your mean is ... i guess your mean the device remove event we get from qmp monitor won't happen ? we will get that event if qemu remove shmem device success, it should always happen if qemu really remove it and there is no bugs on qemu :)
While reviewing I got lazy and didn't check the non hotplug case to how shmem is added to the vm, but the point I was trying to make is that "if (shmem->server.enabled)" fails (e.g. is false), then there is no "rc = qemuMonitorDelObject(priv->mon, objAlias);" call in this API (similar to RNG code), thus how does the following event get triggered? Even if the condition was true, does detaching the char dev cause the event to be triggered? I thought the event was related to the DelObject code, but I didn't go follow that code John [...]
On 12/15/2015 08:39 PM, John Ferlan wrote:
[...]
+ if (rc < 0) + return -1; + I know this is a copy of the RemoveRNGDevice; however, this code doesn't remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter the monitor at all.
Thus the following event probably won't happen... I am not sure what your mean is ... i guess your mean the device remove event we get from qmp monitor won't happen ? we will get that event if qemu remove shmem device success, it should always happen if qemu really remove it and there is no bugs on qemu :)
While reviewing I got lazy and didn't check the non hotplug case to how shmem is added to the vm, but the point I was trying to make is that "if (shmem->server.enabled)" fails (e.g. is false), then there is no "rc = qemuMonitorDelObject(priv->mon, objAlias);" call in this API (similar to RNG code), thus how does the following event get triggered? Even if the condition was true, does detaching the char dev cause the event to be triggered? I thought the event was related to the DelObject code, but I didn't go follow that code
Oh, i see, event is not related to the object delete, i guess you have checked the code and know how it works now ;-) Thanks for your quick reply ! Luyao
John
[...]
On 12/16/2015 09:43 PM, lhuang wrote:
On 12/15/2015 08:39 PM, John Ferlan wrote:
[...]
+ if (rc < 0) + return -1; + I know this is a copy of the RemoveRNGDevice; however, this code doesn't remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter the monitor at all.
Thus the following event probably won't happen... I am not sure what your mean is ... i guess your mean the device remove event we get from qmp monitor won't happen ? we will get that event if qemu remove shmem device success, it should always happen if qemu really remove it and there is no bugs on qemu :)
While reviewing I got lazy and didn't check the non hotplug case to how shmem is added to the vm, but the point I was trying to make is that "if (shmem->server.enabled)" fails (e.g. is false), then there is no "rc = qemuMonitorDelObject(priv->mon, objAlias);" call in this API (similar to RNG code), thus how does the following event get triggered? Even if the condition was true, does detaching the char dev cause the event to be triggered? I thought the event was related to the DelObject code, but I didn't go follow that code
Oh, i see, event is not related to the object delete, i guess you have checked the code and know how it works now ;-)
Maybe you misunderstood - maybe I misunderstood your response... I didn't go searching through the code, but something just didn't seem right when looking so I wanted to note it. Since I figured your model was RNG my thought process was how does RNG do it and what is different... AttachRNG has 3 steps AttachCharDev, AddObject, and AddDevice DetachRNG has 1 step DelDevice RemoveRNG has 2 steps DelObject and DetachCharDev AttachShmem has 2 steps AttachCharDev and AddDevice DetachShmem has 1 step DelDevice RemoveShmem has 1 step DetachCharDev I think my concern was more related to the difference where an RNG has an Object that gets removed, but Shmem doesn't. After closer inspection now it seems that this event generation needs to stay. The RemoveShmem code is generating the event not waiting on it which perhaps is where my thoughts were when I first went through the changes. John
Thanks for your quick reply !
Luyao
John
[...]
On 12/17/2015 08:26 PM, John Ferlan wrote:
On 12/16/2015 09:43 PM, lhuang wrote:
On 12/15/2015 08:39 PM, John Ferlan wrote:
[...]
+ if (rc < 0) + return -1; + I know this is a copy of the RemoveRNGDevice; however, this code doesn't remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter the monitor at all.
Thus the following event probably won't happen... I am not sure what your mean is ... i guess your mean the device remove event we get from qmp monitor won't happen ? we will get that event if qemu remove shmem device success, it should always happen if qemu really remove it and there is no bugs on qemu :)
While reviewing I got lazy and didn't check the non hotplug case to how shmem is added to the vm, but the point I was trying to make is that "if (shmem->server.enabled)" fails (e.g. is false), then there is no "rc = qemuMonitorDelObject(priv->mon, objAlias);" call in this API (similar to RNG code), thus how does the following event get triggered? Even if the condition was true, does detaching the char dev cause the event to be triggered? I thought the event was related to the DelObject code, but I didn't go follow that code Oh, i see, event is not related to the object delete, i guess you have checked the code and know how it works now ;-)
Maybe you misunderstood - maybe I misunderstood your response... I didn't go searching through the code, but something just didn't seem right when looking so I wanted to note it.
Looks like i misunderstood :)
Since I figured your model was RNG my thought process was how does RNG do it and what is different...
AttachRNG has 3 steps AttachCharDev, AddObject, and AddDevice DetachRNG has 1 step DelDevice RemoveRNG has 2 steps DelObject and DetachCharDev
AttachShmem has 2 steps AttachCharDev and AddDevice DetachShmem has 1 step DelDevice RemoveShmem has 1 step DetachCharDev
I think my concern was more related to the difference where an RNG has an Object that gets removed, but Shmem doesn't.
ivshmem device doesn't have a QOM object, so no need to add/del a QOM object. and you can check this doc: http://wiki.qemu.org/TexiDemo and see "Inter-VM Shared Memory device" part
After closer inspection now it seems that this event generation needs to stay. The RemoveShmem code is generating the event not waiting on it which perhaps is where my thoughts were when I first went through the changes.
yes, virDomainEventDeviceRemovedNewFromObj() is generating a event and it can trigger the callback functions which registered in qemu driver. Thanks for your help and reply Luayo
John
Thanks for your quick reply !
Luyao
John
[...]
On 12/21/2015 10:24 AM, lhuang wrote:
On 12/17/2015 08:26 PM, John Ferlan wrote:
On 12/16/2015 09:43 PM, lhuang wrote:
On 12/15/2015 08:39 PM, John Ferlan wrote:
[...]
> + if (rc < 0) > + return -1; > + I know this is a copy of the RemoveRNGDevice; however, this code doesn't remove an 'obj'. In fact, if !shmem->server.enabled, then we don't enter the monitor at all.
Thus the following event probably won't happen... I am not sure what your mean is ... i guess your mean the device remove event we get from qmp monitor won't happen ? we will get that event if qemu remove shmem device success, it should always happen if qemu really remove it and there is no bugs on qemu :)
While reviewing I got lazy and didn't check the non hotplug case to how shmem is added to the vm, but the point I was trying to make is that "if (shmem->server.enabled)" fails (e.g. is false), then there is no "rc = qemuMonitorDelObject(priv->mon, objAlias);" call in this API (similar to RNG code), thus how does the following event get triggered? Even if the condition was true, does detaching the char dev cause the event to be triggered? I thought the event was related to the DelObject code, but I didn't go follow that code Oh, i see, event is not related to the object delete, i guess you have checked the code and know how it works now ;-)
Maybe you misunderstood - maybe I misunderstood your response... I didn't go searching through the code, but something just didn't seem right when looking so I wanted to note it.
Looks like i misunderstood :)
Since I figured your model was RNG my thought process was how does RNG do it and what is different...
AttachRNG has 3 steps AttachCharDev, AddObject, and AddDevice DetachRNG has 1 step DelDevice RemoveRNG has 2 steps DelObject and DetachCharDev
AttachShmem has 2 steps AttachCharDev and AddDevice DetachShmem has 1 step DelDevice RemoveShmem has 1 step DetachCharDev
I think my concern was more related to the difference where an RNG has an Object that gets removed, but Shmem doesn't.
ivshmem device doesn't have a QOM object, so no need to add/del a QOM object.
and you can check this doc:
and see "Inter-VM Shared Memory device" part
ivshmem device could have a object when it use x-memdev, so seems we will add DelObject in RemoveShmem in future :) (maybe when someone implement ivshmem with hugepage support) Luyao
After closer inspection now it seems that this event generation needs to stay. The RemoveShmem code is generating the event not waiting on it which perhaps is where my thoughts were when I first went through the changes.
yes, virDomainEventDeviceRemovedNewFromObj() is generating a event and it can trigger the callback functions which registered in qemu driver.
Thanks for your help and reply
Luayo
John
Thanks for your quick reply !
Luyao
John
[...]
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
John Ferlan -
lhuang -
Luyao Huang