[libvirt] [PATCH 00/12]qemu: support hot-plug/unplug RNG device
qemu already support hot-plug and hot-unplug RNG device. These patch will make libvirt support hot-plug/unplug RNG device for qemu driver. Luyao Huang (12): qemu: introduce a new func qemuAssignDeviceRNGAlias for rng device qemu: rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change something conf: introduce a new func virDomainRNGEquals conf: introduce 3 functions for RNG device libvirt_private: add 4 new func in libvirt_private.syms qemu: add id when build RNG device and rename object id qemu: introduce 2 func qemuDomainRNGInsert and qemuDomainRNGRemove qemu: introduce 2 functions for attach a rng object in json monitor qemu_monitor: add 2 functions qemuMonitorDetachRNGDev and qemuMonitorAttachRNGDev audit: make function virDomainAuditRNG global qemu: Implement RNG device hotplug on live level qemu: Implement RNG device hotunplug on live level src/conf/domain_audit.c | 2 +- src/conf/domain_audit.h | 7 ++ src/conf/domain_conf.c | 78 ++++++++++++++++ src/conf/domain_conf.h | 12 +++ src/libvirt_private.syms | 6 ++ src/qemu/qemu_command.c | 70 +++++++++----- src/qemu/qemu_command.h | 5 + src/qemu/qemu_driver.c | 12 ++- src/qemu/qemu_hotplug.c | 212 ++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 14 ++- src/qemu/qemu_monitor.c | 43 +++++++++ src/qemu/qemu_monitor.h | 7 ++ src/qemu/qemu_monitor_json.c | 58 ++++++++++++ src/qemu/qemu_monitor_json.h | 5 + 14 files changed, 502 insertions(+), 29 deletions(-) -- 1.8.3.1
This function used to set a alias name for RNG device, usage just like other named qemuAssignDevice***Alias functions. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 25 ++++++++++++++++++++++++- src/qemu/qemu_command.h | 1 + 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d5679de..c1e9bca 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -909,6 +909,29 @@ qemuGetNextChrDevIndex(virDomainDefPtr def, return idx; } +int +qemuAssignDeviceRNGAlias(virDomainDefPtr def, virDomainRNGDefPtr rng, int idx) +{ + if (idx == -1) { + size_t i; + idx = 0; + for (i = 0; i < def->nrngs; i++) { + int thisidx; + if ((thisidx = qemuDomainDeviceAliasIndex(&def->rngs[i]->info, "rng")) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine device index for RNG device")); + return -1; + } + if (thisidx >= idx) + idx = thisidx + 1; + } + } + + if (virAsprintf(&rng->info.alias, "rng%d", idx) < 0) + return -1; + + return 0; +} int qemuAssignDeviceChrAlias(virDomainDefPtr def, @@ -1035,7 +1058,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nrngs; i++) { - if (virAsprintf(&def->rngs[i]->info.alias, "rng%zu", i) < 0) + if (qemuAssignDeviceRNGAlias(def, def->rngs[i], i) < 0) return -1; } if (def->tpm) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index f7d3c2d..9f3f249 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -262,6 +262,7 @@ int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr r int qemuAssignDeviceChrAlias(virDomainDefPtr def, virDomainChrDefPtr chr, ssize_t idx); +int qemuAssignDeviceRNGAlias(virDomainDefPtr def, virDomainRNGDefPtr rng, int idx); int qemuParseKeywords(const char *str, -- 1.8.3.1
Subject: perhaps.. qemu: Add helper to assign RNG device aliases On 01/03/15 06:06, Luyao Huang wrote:
This function used to set a alias name for RNG device, usage just
This function is used to assign an alias for a RNG device. It will be later reused when hotplugging RNGs.
like other named qemuAssignDevice***Alias functions.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 25 ++++++++++++++++++++++++- src/qemu/qemu_command.h | 1 + 2 files changed, 25 insertions(+), 1 deletion(-)
ACK with the commit message changes. (Please include the change in the next posting, as I'm already requiring a new version). Peter
On 01/05/2015 11:10 PM, Peter Krempa wrote:
Subject: perhaps..
qemu: Add helper to assign RNG device aliases Thanks, good Subject On 01/03/15 06:06, Luyao Huang wrote:
This function used to set a alias name for RNG device, usage just This function is used to assign an alias for a RNG device. It will be later reused when hotplugging RNGs. Thanks a lot like other named qemuAssignDevice***Alias functions.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 25 ++++++++++++++++++++++++- src/qemu/qemu_command.h | 1 + 2 files changed, 25 insertions(+), 1 deletion(-)
ACK with the commit message changes. (Please include the change in the next posting, as I'm already requiring a new version).
Peter
rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr, we need this function to build a cmdline. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 33 ++++++++++++++++----------------- src/qemu/qemu_command.h | 4 ++++ 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c1e9bca..46e289d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5800,22 +5800,19 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, return ret; } - -static int -qemuBuildRNGDeviceArgs(virCommandPtr cmd, - virDomainDefPtr def, - virDomainRNGDefPtr dev, - virQEMUCapsPtr qemuCaps) +char * +qemuBuildRNGDevStr(virDomainDefPtr def, + virDomainRNGDefPtr dev, + virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; - int ret = -1; if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("this qemu doesn't support RNG device type '%s'"), virDomainRNGModelTypeToString(dev->model)); - goto cleanup; + goto error; } if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) @@ -5836,16 +5833,14 @@ qemuBuildRNGDeviceArgs(virCommandPtr cmd, } if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) - goto cleanup; - - virCommandAddArg(cmd, "-device"); - virCommandAddArgBuffer(cmd, &buf); - - ret = 0; + goto error; + if (virBufferCheckError(&buf) < 0) + goto error; - cleanup: + return virBufferContentAndReset(&buf); + error: virBufferFreeAndReset(&buf); - return ret; + return NULL; } @@ -9802,13 +9797,17 @@ qemuBuildCommandLine(virConnectPtr conn, } for (i = 0; i < def->nrngs; i++) { + char *devstr; /* add the RNG source backend */ if (qemuBuildRNGBackendArgs(cmd, def->rngs[i], qemuCaps) < 0) goto error; /* add the device */ - if (qemuBuildRNGDeviceArgs(cmd, def, def->rngs[i], qemuCaps) < 0) + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildRNGDevStr(def, def->rngs[i], qemuCaps))) goto error; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } if (def->nvram) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 9f3f249..4264215 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -163,6 +163,10 @@ char *qemuBuildPCIHostdevDevStr(virDomainDefPtr def, const char *configfd, virQEMUCapsPtr qemuCaps); +char *qemuBuildRNGDevStr(virDomainDefPtr def, + virDomainRNGDefPtr dev, + virQEMUCapsPtr qemuCaps); + int qemuOpenPCIConfig(virDomainHostdevDefPtr dev); /* Legacy, pre device support */ -- 1.8.3.1
Subject: "change something"? That's a really vague statement. How about: qemu: refactor qemuBuildRNGDeviceArgs to allow reuse in RNG hotplug On 01/03/15 06:06, Luyao Huang wrote:
rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr, we need this function to build a cmdline.
Rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change the return type so that it can be reused in the device hotplug code later.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 33 ++++++++++++++++----------------- src/qemu/qemu_command.h | 4 ++++ 2 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c1e9bca..46e289d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5800,22 +5800,19 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, return ret; }
-
Don't delete the line. Functions are usually separated by two newlines.
-static int -qemuBuildRNGDeviceArgs(virCommandPtr cmd, - virDomainDefPtr def, - virDomainRNGDefPtr dev, - virQEMUCapsPtr qemuCaps) +char * +qemuBuildRNGDevStr(virDomainDefPtr def, + virDomainRNGDefPtr dev, + virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; - int ret = -1;
if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("this qemu doesn't support RNG device type '%s'"), virDomainRNGModelTypeToString(dev->model)); - goto cleanup; + goto error; }
if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)
ACK, looks good except for the commit message and the spurious line deletion. Peter
On 01/05/2015 11:18 PM, Peter Krempa wrote:
Subject: "change something"? That's a really vague statement.
How about:
qemu: refactor qemuBuildRNGDeviceArgs to allow reuse in RNG hotplug Good subject :) Thanks On 01/03/15 06:06, Luyao Huang wrote:
rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr, we need this function to build a cmdline. Rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change the return type so that it can be reused in the device hotplug code later.
Thanks
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 33 ++++++++++++++++----------------- src/qemu/qemu_command.h | 4 ++++ 2 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c1e9bca..46e289d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5800,22 +5800,19 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, return ret; }
- Don't delete the line. Functions are usually separated by two newlines. Okay -static int -qemuBuildRNGDeviceArgs(virCommandPtr cmd, - virDomainDefPtr def, - virDomainRNGDefPtr dev, - virQEMUCapsPtr qemuCaps) +char * +qemuBuildRNGDevStr(virDomainDefPtr def, + virDomainRNGDefPtr dev, + virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; - int ret = -1;
if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("this qemu doesn't support RNG device type '%s'"), virDomainRNGModelTypeToString(dev->model)); - goto cleanup; + goto error; }
if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) ACK, looks good except for the commit message and the spurious line deletion.
Peter
On 01/05/2015 11:56 PM, lhuang wrote:
On 01/05/2015 11:18 PM, Peter Krempa wrote:
Subject: "change something"? That's a really vague statement.
How about:
qemu: refactor qemuBuildRNGDeviceArgs to allow reuse in RNG hotplug Good subject :) Thanks On 01/03/15 06:06, Luyao Huang wrote:
rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr, we need this function to build a cmdline. Rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change the return type so that it can be reused in the device hotplug code later.
Thanks
Signed-off-by: Luyao Huang <lhuang@redhat.com>
Another hint for more efficient email: It's a bit hard to pick out your replies from the quoted text via a visual scan, once another layer of quoting is added. A good rule of thumb to follow is to include a blank line before and after every section of your new text that you intersperse when replying to quoted text. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/07/2015 01:16 AM, Eric Blake wrote:
On 01/05/2015 11:18 PM, Peter Krempa wrote:
Subject: "change something"? That's a really vague statement.
How about:
qemu: refactor qemuBuildRNGDeviceArgs to allow reuse in RNG hotplug Good subject :) Thanks On 01/03/15 06:06, Luyao Huang wrote:
rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr, we need this function to build a cmdline. Rename qemuBuildRNGDeviceArgs to qemuBuildRNGDevStr and change the return type so that it can be reused in the device hotplug code later.
Thanks
Signed-off-by: Luyao Huang <lhuang@redhat.com> Another hint for more efficient email: It's a bit hard to pick out your replies from the quoted text via a visual scan, once another layer of quoting is added. A good rule of thumb to follow is to include a blank
On 01/05/2015 11:56 PM, lhuang wrote: line before and after every section of your new text that you intersperse when replying to quoted text.
Thanks for your advise. I add a blank line this time (also next times), I think it will be more easy to find my reply. Luyao
virDomainRNGEquals is a func which check if two rng device are the same. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 34 ++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ 2 files changed, 37 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aafc05e..91c114e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11922,6 +11922,40 @@ virDomainChrRemove(virDomainDefPtr vmdef, return ret; } +bool +virDomainRNGEquals(virDomainRNGDefPtr src, + virDomainRNGDefPtr tgt) +{ + if (!src || !tgt) + return src == tgt; + + if (src->model != tgt->model) + return false; + + if (src->rate != tgt->rate || src->period != tgt->period) + return false; + + switch ((virDomainRNGModel) src->model) { + case VIR_DOMAIN_RNG_MODEL_VIRTIO: + switch ((virDomainRNGBackend) src->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + return STREQ_NULLABLE(src->source.file, tgt->source.file); + break; + case VIR_DOMAIN_RNG_BACKEND_EGD: + return virDomainChrSourceDefIsEqual((virDomainChrSourceDef *) src->source.chardev, + (virDomainChrSourceDef *) tgt->source.chardev); + break; + case VIR_DOMAIN_RNG_BACKEND_LAST: + break; + } + break; + + case VIR_DOMAIN_RNG_MODEL_LAST: + break; + } + return false; +} + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 57297cd..c197095 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2597,6 +2597,9 @@ virDomainChrInsert(virDomainDefPtr vmdef, virDomainChrDefPtr virDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr); +bool +virDomainRNGEquals(virDomainRNGDefPtr src, + virDomainRNGDefPtr tgt); int virDomainSaveXML(const char *configDir, virDomainDefPtr def, -- 1.8.3.1
Subject: ... How about conf: Introduce function to compare RNG devices. On 01/03/15 06:06, Luyao Huang wrote:
virDomainRNGEquals is a func which check if two rng device are the same.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 34 ++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ 2 files changed, 37 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aafc05e..91c114e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11922,6 +11922,40 @@ virDomainChrRemove(virDomainDefPtr vmdef, return ret; }
+bool +virDomainRNGEquals(virDomainRNGDefPtr src, + virDomainRNGDefPtr tgt) +{ + if (!src || !tgt) + return src == tgt; + + if (src->model != tgt->model) + return false; + + if (src->rate != tgt->rate || src->period != tgt->period) + return false; + + switch ((virDomainRNGModel) src->model) { + case VIR_DOMAIN_RNG_MODEL_VIRTIO: + switch ((virDomainRNGBackend) src->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + return STREQ_NULLABLE(src->source.file, tgt->source.file); + break; + case VIR_DOMAIN_RNG_BACKEND_EGD: + return virDomainChrSourceDefIsEqual((virDomainChrSourceDef *) src->source.chardev, + (virDomainChrSourceDef *) tgt->source.chardev);
No need for typecast here; both src->source.chardev and tgt ... are already in the required type.
+ break; + case VIR_DOMAIN_RNG_BACKEND_LAST: + break; + } + break; + + case VIR_DOMAIN_RNG_MODEL_LAST: + break; + } + return false; +} + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 57297cd..c197095 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2597,6 +2597,9 @@ virDomainChrInsert(virDomainDefPtr vmdef, virDomainChrDefPtr virDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr); +bool +virDomainRNGEquals(virDomainRNGDefPtr src, + virDomainRNGDefPtr tgt);
As in the case below, please add the type to the same line as the declaration.
int virDomainSaveXML(const char *configDir, virDomainDefPtr def,
Peter
On 01/05/2015 11:00 PM, Peter Krempa wrote:
Subject: ... How about
conf: Introduce function to compare RNG devices.
On 01/03/15 06:06, Luyao Huang wrote:
virDomainRNGEquals is a func which check if two rng device are the same.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 34 ++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ 2 files changed, 37 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aafc05e..91c114e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11922,6 +11922,40 @@ virDomainChrRemove(virDomainDefPtr vmdef, return ret; }
+bool +virDomainRNGEquals(virDomainRNGDefPtr src, + virDomainRNGDefPtr tgt) +{ + if (!src || !tgt) + return src == tgt; + + if (src->model != tgt->model) + return false; + + if (src->rate != tgt->rate || src->period != tgt->period) + return false; + + switch ((virDomainRNGModel) src->model) { + case VIR_DOMAIN_RNG_MODEL_VIRTIO: + switch ((virDomainRNGBackend) src->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + return STREQ_NULLABLE(src->source.file, tgt->source.file); + break; + case VIR_DOMAIN_RNG_BACKEND_EGD: + return virDomainChrSourceDefIsEqual((virDomainChrSourceDef *) src->source.chardev, + (virDomainChrSourceDef *) tgt->source.chardev); No need for typecast here; both src->source.chardev and tgt ... are already in the required type.
Got it, i will remove the typecast in next version.
+ break; + case VIR_DOMAIN_RNG_BACKEND_LAST: + break; + } + break; + + case VIR_DOMAIN_RNG_MODEL_LAST: + break; + } + return false; +} + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 57297cd..c197095 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2597,6 +2597,9 @@ virDomainChrInsert(virDomainDefPtr vmdef, virDomainChrDefPtr virDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr); +bool +virDomainRNGEquals(virDomainRNGDefPtr src, + virDomainRNGDefPtr tgt); As in the case below, please add the type to the same line as the declaration. OKay
Thanks for your review
int virDomainSaveXML(const char *configDir, virDomainDefPtr def,
Peter
the 3 functions are: virDomainRNGInsert: Insert a RNG device to vm->def. virDomainRNGRemove: remove a RNG device in vm->def. virDomainRNGFind: find a RNG device in vm->def. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++++ 2 files changed, 53 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 91c114e..37c4569 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11956,6 +11956,50 @@ virDomainRNGEquals(virDomainRNGDefPtr src, return false; } +int +virDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + return VIR_APPEND_ELEMENT(vmdef->rngs, vmdef->nrngs, rng); +} + +virDomainRNGDefPtr +virDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + size_t i; + + for (i = 0; i < vmdef->nrngs; i++) { + ret = vmdef->rngs[i]; + + if (virDomainRNGEquals(ret, rng)) + break; + } + + if (i == vmdef->nrngs) + return NULL; + + VIR_DELETE_ELEMENT(vmdef->rngs, i, vmdef->nrngs); + return ret; +} + +virDomainRNGDefPtr +virDomainRNGFind(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + size_t i; + + for (i = 0; i < vmdef->nrngs; i++) { + ret = vmdef->rngs[i]; + + if (virDomainRNGEquals(ret, rng)) + return ret; + } + return NULL; +} + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c197095..cb87fad 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2600,6 +2600,15 @@ virDomainChrRemove(virDomainDefPtr vmdef, bool virDomainRNGEquals(virDomainRNGDefPtr src, virDomainRNGDefPtr tgt); +int +virDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); +virDomainRNGDefPtr +virDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); +virDomainRNGDefPtr +virDomainRNGFind(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); int virDomainSaveXML(const char *configDir, virDomainDefPtr def, -- 1.8.3.1
The subject is vague. Please try to condense what the patch is adding in a way that describes the functions instead of an opaque "add 3 functions" On 01/03/15 06:06, Luyao Huang wrote:
the 3 functions are: virDomainRNGInsert: Insert a RNG device to vm->def. virDomainRNGRemove: remove a RNG device in vm->def. virDomainRNGFind: find a RNG device in vm->def.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++++ 2 files changed, 53 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 91c114e..37c4569 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11956,6 +11956,50 @@ virDomainRNGEquals(virDomainRNGDefPtr src, return false; }
+int +virDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + return VIR_APPEND_ELEMENT(vmdef->rngs, vmdef->nrngs, rng); +} + +virDomainRNGDefPtr +virDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + size_t i; + + for (i = 0; i < vmdef->nrngs; i++) { + ret = vmdef->rngs[i]; + + if (virDomainRNGEquals(ret, rng))
If you add a block here and move the VIR_DELETE_ELEMENT line here.
+ break;
And return ret instead of breaking here.
+ }
Then you can "return NULL" here and get rid of the rest. The resulting code will be more readable.
+ + if (i == vmdef->nrngs) + return NULL; + + VIR_DELETE_ELEMENT(vmdef->rngs, i, vmdef->nrngs); + return ret; +} + +virDomainRNGDefPtr +virDomainRNGFind(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + size_t i; + + for (i = 0; i < vmdef->nrngs; i++) { + ret = vmdef->rngs[i]; + + if (virDomainRNGEquals(ret, rng)) + return ret; + } + return NULL; +} + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c197095..cb87fad 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2600,6 +2600,15 @@ virDomainChrRemove(virDomainDefPtr vmdef, bool virDomainRNGEquals(virDomainRNGDefPtr src, virDomainRNGDefPtr tgt); +int +virDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng);
In header files we usually put the return type on the same line as the declaration.
+virDomainRNGDefPtr +virDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); +virDomainRNGDefPtr +virDomainRNGFind(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng);
int virDomainSaveXML(const char *configDir, virDomainDefPtr def,
Like here.
Peter
On 01/05/2015 10:56 PM, Peter Krempa wrote:
The subject is vague. Please try to condense what the patch is adding in a way that describes the functions instead of an opaque "add 3 functions"
On 01/03/15 06:06, Luyao Huang wrote:
the 3 functions are: virDomainRNGInsert: Insert a RNG device to vm->def. virDomainRNGRemove: remove a RNG device in vm->def. virDomainRNGFind: find a RNG device in vm->def.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++++++++ 2 files changed, 53 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 91c114e..37c4569 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11956,6 +11956,50 @@ virDomainRNGEquals(virDomainRNGDefPtr src, return false; }
+int +virDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + return VIR_APPEND_ELEMENT(vmdef->rngs, vmdef->nrngs, rng); +} + +virDomainRNGDefPtr +virDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + size_t i; + + for (i = 0; i < vmdef->nrngs; i++) { + ret = vmdef->rngs[i]; + + if (virDomainRNGEquals(ret, rng)) If you add a block here and move the VIR_DELETE_ELEMENT line here.
+ break; And return ret instead of breaking here.
+ } Then you can "return NULL" here and get rid of the rest.
The resulting code will be more readable.
Good idea, thanks a lot.
+ + if (i == vmdef->nrngs) + return NULL; + + VIR_DELETE_ELEMENT(vmdef->rngs, i, vmdef->nrngs); + return ret; +} + +virDomainRNGDefPtr +virDomainRNGFind(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + size_t i; + + for (i = 0; i < vmdef->nrngs; i++) { + ret = vmdef->rngs[i]; + + if (virDomainRNGEquals(ret, rng)) + return ret; + } + return NULL; +} + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c197095..cb87fad 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2600,6 +2600,15 @@ virDomainChrRemove(virDomainDefPtr vmdef, bool virDomainRNGEquals(virDomainRNGDefPtr src, virDomainRNGDefPtr tgt); +int +virDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); In header files we usually put the return type on the same line as the declaration.
Okay, thanks , i will change them in next version
+virDomainRNGDefPtr +virDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); +virDomainRNGDefPtr +virDomainRNGFind(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng);
int virDomainSaveXML(const char *configDir, virDomainDefPtr def, Like here.
Peter
Luyao
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa776b4..deab4cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -375,7 +375,12 @@ virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; virDomainRNGBackendTypeToString; +virDomainRNGDefFree; +virDomainRNGEquals; +virDomainRNGFind; +virDomainRNGInsert; virDomainRNGModelTypeToString; +virDomainRNGRemove; virDomainRunningReasonTypeFromString; virDomainRunningReasonTypeToString; virDomainSaveConfig; -- 1.8.3.1
On 01/03/15 06:06, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa776b4..deab4cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -375,7 +375,12 @@ virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; virDomainRNGBackendTypeToString; +virDomainRNGDefFree; +virDomainRNGEquals; +virDomainRNGFind; +virDomainRNGInsert; virDomainRNGModelTypeToString; +virDomainRNGRemove; virDomainRunningReasonTypeFromString; virDomainRunningReasonTypeToString; virDomainSaveConfig;
These should be in the patches that add the individual functions. Peter
On 01/03/15 06:06, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa776b4..deab4cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -375,7 +375,12 @@ virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; virDomainRNGBackendTypeToString; +virDomainRNGDefFree; +virDomainRNGEquals; +virDomainRNGFind; +virDomainRNGInsert; virDomainRNGModelTypeToString; +virDomainRNGRemove; virDomainRunningReasonTypeFromString; virDomainRunningReasonTypeToString; virDomainSaveConfig;
These should be in the patches that add the individual functions. Okay, i will move virDomainRNGEquals, virDomainRNGFind, virDomainRNGEquals, virDomainRNGRemove to other
On 01/05/2015 10:48 PM, Peter Krempa wrote: patchs, and leave virDomainRNGDefFree in this patch. Thanks for review.
Peter
On 01/06/15 09:10, lhuang wrote:
On 01/03/15 06:06, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa776b4..deab4cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -375,7 +375,12 @@ virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; virDomainRNGBackendTypeToString; +virDomainRNGDefFree; +virDomainRNGEquals; +virDomainRNGFind; +virDomainRNGInsert; virDomainRNGModelTypeToString; +virDomainRNGRemove; virDomainRunningReasonTypeFromString; virDomainRunningReasonTypeToString; virDomainSaveConfig;
These should be in the patches that add the individual functions. Okay, i will move virDomainRNGEquals, virDomainRNGFind, virDomainRNGEquals, virDomainRNGRemove to other
On 01/05/2015 10:48 PM, Peter Krempa wrote: patchs, and leave virDomainRNGDefFree in this patch.
If a function exists already, then add the symbol export to the patch that uses it in a place that requires the symbol being exported.
Thanks for review.
Peter
Peter
On 01/06/2015 04:11 PM, Peter Krempa wrote:
On 01/03/15 06:06, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index aa776b4..deab4cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -375,7 +375,12 @@ virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; virDomainRNGBackendTypeToString; +virDomainRNGDefFree; +virDomainRNGEquals; +virDomainRNGFind; +virDomainRNGInsert; virDomainRNGModelTypeToString; +virDomainRNGRemove; virDomainRunningReasonTypeFromString; virDomainRunningReasonTypeToString; virDomainSaveConfig;
These should be in the patches that add the individual functions. Okay, i will move virDomainRNGEquals, virDomainRNGFind, virDomainRNGEquals, virDomainRNGRemove to other
On 01/05/2015 10:48 PM, Peter Krempa wrote: patchs, and leave virDomainRNGDefFree in this patch. If a function exists already, then add the symbol export to the patch
On 01/06/15 09:10, lhuang wrote: that uses it in a place that requires the symbol being exported. Got it, thanks your advise.
Thanks for review.
Peter
Peter
Luyao
We didn't set a id when we build RNG device cmdline before. Give a id to every RNG device and we can hotunplug it via QMP cmd device_del. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 46e289d..a4073ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5761,7 +5761,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, goto cleanup; } - virBufferAsprintf(&buf, "rng-random,id=%s,filename=%s", + virBufferAsprintf(&buf, "rng-random,id=obj%s,filename=%s", dev->info.alias, dev->source.file); virCommandAddArg(cmd, "-object"); @@ -5784,7 +5784,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, virCommandAddArgList(cmd, "-chardev", backend, NULL); virCommandAddArg(cmd, "-object"); - virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=%s", + virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=obj%s", dev->info.alias, dev->info.alias); break; @@ -5816,13 +5816,13 @@ qemuBuildRNGDevStr(virDomainDefPtr def, } if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else - virBufferAsprintf(&buf, "virtio-rng-pci,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-pci,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); if (dev->rate > 0) { virBufferAsprintf(&buf, ",max-bytes=%u", dev->rate); -- 1.8.3.1
On 01/03/15 06:06, Luyao Huang wrote:
We didn't set a id when we build RNG device cmdline before. Give a id to every RNG device and we can hotunplug it via QMP cmd device_del.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 46e289d..a4073ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5761,7 +5761,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, goto cleanup; }
- virBufferAsprintf(&buf, "rng-random,id=%s,filename=%s", + virBufferAsprintf(&buf, "rng-random,id=obj%s,filename=%s", dev->info.alias, dev->source.file);
virCommandAddArg(cmd, "-object"); @@ -5784,7 +5784,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, virCommandAddArgList(cmd, "-chardev", backend, NULL);
virCommandAddArg(cmd, "-object"); - virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=%s", + virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=obj%s", dev->info.alias, dev->info.alias); break;
@@ -5816,13 +5816,13 @@ qemuBuildRNGDevStr(virDomainDefPtr def, }
if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else - virBufferAsprintf(&buf, "virtio-rng-pci,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-pci,rng=obj%s,id=%s", dev->info.alias, dev->info.alias);
if (dev->rate > 0) { virBufferAsprintf(&buf, ",max-bytes=%u", dev->rate);
This breaks the testsuite as you didn't fix the expected outputs after such a change. Please always run "make check" before posting patches. Additional question. Is this change even necessary? The RNG device does have it's alias already whithout the "obj" prefix ... thus it's just "rng0" for example. NACK unless you have a good reason for the change. Peter
On 01/05/15 15:51, Peter Krempa wrote:
On 01/03/15 06:06, Luyao Huang wrote:
We didn't set a id when we build RNG device cmdline before. Give a id to every RNG device and we can hotunplug it via QMP cmd device_del.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 46e289d..a4073ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5761,7 +5761,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, goto cleanup; }
- virBufferAsprintf(&buf, "rng-random,id=%s,filename=%s", + virBufferAsprintf(&buf, "rng-random,id=obj%s,filename=%s", dev->info.alias, dev->source.file);
virCommandAddArg(cmd, "-object"); @@ -5784,7 +5784,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, virCommandAddArgList(cmd, "-chardev", backend, NULL);
virCommandAddArg(cmd, "-object"); - virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=%s", + virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=obj%s", dev->info.alias, dev->info.alias); break;
@@ -5816,13 +5816,13 @@ qemuBuildRNGDevStr(virDomainDefPtr def, }
if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else - virBufferAsprintf(&buf, "virtio-rng-pci,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-pci,rng=obj%s,id=%s", dev->info.alias, dev->info.alias);
if (dev->rate > 0) { virBufferAsprintf(&buf, ",max-bytes=%u", dev->rate);
This breaks the testsuite as you didn't fix the expected outputs after such a change. Please always run "make check" before posting patches.
Additional question. Is this change even necessary? The RNG device does have it's alias already whithout the "obj" prefix ... thus it's just "rng0" for example.
I misread the code. This is actually necessary as otherwise the -device would have the same ID as the backend object. That makes sense to change, although we need to make sure then that the code will work in case of a long running VM (with the incorrect name) and a new libvirt instance. At any rate ... you need to fix the tests after this commit
Peter
On 01/05/2015 11:45 PM, Peter Krempa wrote:
On 01/05/15 15:51, Peter Krempa wrote:
On 01/03/15 06:06, Luyao Huang wrote:
We didn't set a id when we build RNG device cmdline before. Give a id to every RNG device and we can hotunplug it via QMP cmd device_del.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 46e289d..a4073ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5761,7 +5761,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, goto cleanup; }
- virBufferAsprintf(&buf, "rng-random,id=%s,filename=%s", + virBufferAsprintf(&buf, "rng-random,id=obj%s,filename=%s", dev->info.alias, dev->source.file);
virCommandAddArg(cmd, "-object"); @@ -5784,7 +5784,7 @@ qemuBuildRNGBackendArgs(virCommandPtr cmd, virCommandAddArgList(cmd, "-chardev", backend, NULL);
virCommandAddArg(cmd, "-object"); - virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=%s", + virCommandAddArgFormat(cmd, "rng-egd,chardev=char%s,id=obj%s", dev->info.alias, dev->info.alias); break;
@@ -5816,13 +5816,13 @@ qemuBuildRNGDevStr(virDomainDefPtr def, }
if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", dev->info.alias, dev->info.alias); else - virBufferAsprintf(&buf, "virtio-rng-pci,rng=%s", dev->info.alias); + virBufferAsprintf(&buf, "virtio-rng-pci,rng=obj%s,id=%s", dev->info.alias, dev->info.alias);
if (dev->rate > 0) { virBufferAsprintf(&buf, ",max-bytes=%u", dev->rate);
This breaks the testsuite as you didn't fix the expected outputs after such a change. Please always run "make check" before posting patches.
Additional question. Is this change even necessary? The RNG device does have it's alias already whithout the "obj" prefix ... thus it's just "rng0" for example. I misread the code. This is actually necessary as otherwise the -device would have the same ID as the backend object. That makes sense to change, although we need to make sure then that the code will work in case of a long running VM (with the incorrect name) and a new libvirt instance.
At any rate ... you need to fix the tests after this commit Thanks for your review. Yes, I will give another commit for the tests fix in next version.
I have test with a long running VM (start in old libvirt which RNG device no device name), and update to a libvirt which have these code, if we try to hot-unplug the rng device, qemu will return a error like this : internal error: unable to execute QEMU command 'device_del': Device 'rng0' not found maybe my qemu is too old ? vm qemu cmdline -device do not have a id (because i start it in the old libvirt), so this error is correct in this place. But i don't know how can i hot-unplug a device without a id.
Peter
Luyao
qemu side functions, call virDomainRNGInsert and virDomainRNGRemove to help us. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_hotplug.c | 23 +++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 7 +++++++ 2 files changed, 30 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7f93b9b..f9327b4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1501,6 +1501,29 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, return ret; } +int +qemuDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + return virDomainRNGInsert(vmdef, rng); +} + +virDomainRNGDefPtr +qemuDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + + if (!(ret = virDomainRNGRemove(vmdef, rng))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("device not present in domain configuration")); + return NULL; + } + + return ret; +} + + static int qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index d13c532..7b838ee 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -107,6 +107,13 @@ virDomainChrDefPtr qemuDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr); +int +qemuDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); +virDomainRNGDefPtr +qemuDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); + void qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev); -- 1.8.3.1
On 01/03/15 06:06, Luyao Huang wrote:
qemu side functions, call virDomainRNGInsert and virDomainRNGRemove to help us.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_hotplug.c | 23 +++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 7 +++++++ 2 files changed, 30 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7f93b9b..f9327b4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1501,6 +1501,29 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, return ret; }
+int +qemuDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + return virDomainRNGInsert(vmdef, rng); +}
This wrapper doesn't seem useful.
+ +virDomainRNGDefPtr +qemuDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + + if (!(ret = virDomainRNGRemove(vmdef, rng))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("device not present in domain configuration")); + return NULL; + }
Given that this function is used exactly once in the series you've posted it doesn't make much sense to have the code separate.
+ + return ret; +} + + static int qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index d13c532..7b838ee 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -107,6 +107,13 @@ virDomainChrDefPtr qemuDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr);
+int +qemuDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); +virDomainRNGDefPtr +qemuDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); +
Both of the functions above are used only in qemu_hotplug.c. It doesn't make sense to export them.
void qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev);
Peter
On 01/05/2015 11:22 PM, Peter Krempa wrote:
On 01/03/15 06:06, Luyao Huang wrote:
qemu side functions, call virDomainRNGInsert and virDomainRNGRemove to help us.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_hotplug.c | 23 +++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 7 +++++++ 2 files changed, 30 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7f93b9b..f9327b4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1501,6 +1501,29 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, return ret; }
+int +qemuDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + return virDomainRNGInsert(vmdef, rng); +} This wrapper doesn't seem useful. Yes, i will remove this function in next version.
+ +virDomainRNGDefPtr +qemuDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng) +{ + virDomainRNGDefPtr ret; + + if (!(ret = virDomainRNGRemove(vmdef, rng))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("device not present in domain configuration")); + return NULL; + } Given that this function is used exactly once in the series you've posted it doesn't make much sense to have the code separate.
Okay, ...
+ + return ret; +} + + static int qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index d13c532..7b838ee 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -107,6 +107,13 @@ virDomainChrDefPtr qemuDomainChrRemove(virDomainDefPtr vmdef, virDomainChrDefPtr chr);
+int +qemuDomainRNGInsert(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); +virDomainRNGDefPtr +qemuDomainRNGRemove(virDomainDefPtr vmdef, + virDomainRNGDefPtr rng); + Both of the functions above are used only in qemu_hotplug.c. It doesn't make sense to export them. ... i will remove them and won't export qemuDomainRNGRemove.
Thanks for your review and pointing out.
void qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev);
Peter
We need a new function to build a RNG device object, and need a function to build a props which will be used in qemuMonitorJSONAddObject. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_monitor_json.c | 58 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ 2 files changed, 63 insertions(+) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e567aa7..4430819 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6166,6 +6166,64 @@ qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon, return ret; } +static virJSONValuePtr +qemuMonitorJSONRNGPropsCommand(const char *name, + const char *data) +{ + virJSONValuePtr ret; + + if (!(ret = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(ret, name, data) < 0) + goto error; + + return ret; + + error: + virJSONValueFree(ret); + return NULL; +} + +int +qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon, + const char *chrID, + const char *objID, + virDomainRNGDefPtr rng) +{ + const char *type = NULL; + virJSONValuePtr props = NULL; + + switch ((virDomainRNGBackend) rng->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + type = "rng-random"; + if (!(props = qemuMonitorJSONRNGPropsCommand("filename", rng->source.file))) + goto cleanup; + break; + + case VIR_DOMAIN_RNG_BACKEND_EGD: + if (!chrID) { + virReportError(VIR_ERR_INTERNAL_ERROR,"%s", + _("miss chardev id")); + goto cleanup; + } + type = "rng-egd"; + if (!(props = qemuMonitorJSONRNGPropsCommand("chardev", chrID))) + goto cleanup; + break; + + case VIR_DOMAIN_RNG_BACKEND_LAST: + /*shouldn't happen*/ + goto cleanup; + } + + return qemuMonitorJSONAddObject(mon, type, objID, props); + + cleanup: + virJSONValueFree(props); + return -1; +} + int qemuMonitorJSONGetDeviceAliases(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 222f11e..66c519d 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -459,6 +459,11 @@ int qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, virDomainChrSourceDefPtr chr); int qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon, const char *chrID); +int qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon, + const char *chrID, + const char *objID, + virDomainRNGDefPtr rng); + int qemuMonitorJSONGetDeviceAliases(qemuMonitorPtr mon, char ***aliases); -- 1.8.3.1
On 01/03/15 06:06, Luyao Huang wrote:
We need a new function to build a RNG device object, and need a function to build a props which will be used in qemuMonitorJSONAddObject.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_monitor_json.c | 58 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ 2 files changed, 63 insertions(+)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e567aa7..4430819 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6166,6 +6166,64 @@ qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon, return ret; }
+static virJSONValuePtr +qemuMonitorJSONRNGPropsCommand(const char *name, + const char *data) +{ + virJSONValuePtr ret; + + if (!(ret = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(ret, name, data) < 0) + goto error; + + return ret; + + error: + virJSONValueFree(ret); + return NULL; +}
To allow adding generic properties to objects I've added the virJSONValueObjectCreate function that allows to create generic json value objects. Please use that func instead of the above.
+ +int +qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon, + const char *chrID, + const char *objID, + virDomainRNGDefPtr rng) +{ + const char *type = NULL; + virJSONValuePtr props = NULL; + + switch ((virDomainRNGBackend) rng->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + type = "rng-random"; + if (!(props = qemuMonitorJSONRNGPropsCommand("filename", rng->source.file)))
With usage of virJSONValueObjectCreate the code will look like: if (!(props = virJSONValueObjectCreate("s:filename", rng->source.file, NULL)))
+ goto cleanup; + break; + + case VIR_DOMAIN_RNG_BACKEND_EGD: + if (!chrID) { + virReportError(VIR_ERR_INTERNAL_ERROR,"%s", + _("miss chardev id")); + goto cleanup; + }
The chardev and backend object ID can (and should) be inferred from the rng device ID as they should be the same (except for the "char"/"obj" prefix).
+ type = "rng-egd"; + if (!(props = qemuMonitorJSONRNGPropsCommand("chardev", chrID))) + goto cleanup; + break; + + case VIR_DOMAIN_RNG_BACKEND_LAST: + /*shouldn't happen*/ + goto cleanup; + } + + return qemuMonitorJSONAddObject(mon, type, objID, props); + + cleanup: + virJSONValueFree(props); + return -1; +} +
int
Peter
On 01/05/2015 11:29 PM, Peter Krempa wrote:
On 01/03/15 06:06, Luyao Huang wrote:
We need a new function to build a RNG device object, and need a function to build a props which will be used in qemuMonitorJSONAddObject.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_monitor_json.c | 58 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ 2 files changed, 63 insertions(+)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e567aa7..4430819 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6166,6 +6166,64 @@ qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon, return ret; }
+static virJSONValuePtr +qemuMonitorJSONRNGPropsCommand(const char *name, + const char *data) +{ + virJSONValuePtr ret; + + if (!(ret = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(ret, name, data) < 0) + goto error; + + return ret; + + error: + virJSONValueFree(ret); + return NULL; +} To allow adding generic properties to objects I've added the virJSONValueObjectCreate function that allows to create generic json value objects. Please use that func instead of the above. Thanks for pointing out, i forgot double check the exist functions.
+ +int +qemuMonitorJSONAttachRNGDev(qemuMonitorPtr mon, + const char *chrID, + const char *objID, + virDomainRNGDefPtr rng) +{ + const char *type = NULL; + virJSONValuePtr props = NULL; + + switch ((virDomainRNGBackend) rng->backend) { + case VIR_DOMAIN_RNG_BACKEND_RANDOM: + type = "rng-random"; + if (!(props = qemuMonitorJSONRNGPropsCommand("filename", rng->source.file))) With usage of virJSONValueObjectCreate the code will look like:
if (!(props = virJSONValueObjectCreate("s:filename", rng->source.file, NULL))) Thanks the example, i will use them in next version.
+ goto cleanup; + break; + + case VIR_DOMAIN_RNG_BACKEND_EGD: + if (!chrID) { + virReportError(VIR_ERR_INTERNAL_ERROR,"%s", + _("miss chardev id")); + goto cleanup; + } The chardev and backend object ID can (and should) be inferred from the rng device ID as they should be the same (except for the "char"/"obj" prefix). Eww, i think your mean is add a check for charname and objname, if they are different then output a error?
+ type = "rng-egd"; + if (!(props = qemuMonitorJSONRNGPropsCommand("chardev", chrID))) + goto cleanup; + break; + + case VIR_DOMAIN_RNG_BACKEND_LAST: + /*shouldn't happen*/ + goto cleanup; + } + + return qemuMonitorJSONAddObject(mon, type, objID, props); + + cleanup: + virJSONValueFree(props); + return -1; +} +
int Peter
Luyao
These 2 functions just do some basic check and then call qemuMonitorJSONAttachRNGDev and qemuMonitorDelObject to help us. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_monitor.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 7 +++++++ 2 files changed, 50 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 100bbd0..9ac5934 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4140,6 +4140,49 @@ int qemuMonitorDetachCharDev(qemuMonitorPtr mon, return qemuMonitorJSONDetachCharDev(mon, chrID); } +int qemuMonitorAttachRNGDev(qemuMonitorPtr mon, + const char *chrID, + const char *objID, + virDomainRNGDefPtr rng) +{ + VIR_DEBUG("mon=%p chrID=%s objID=%s rng=%p", mon, chrID, objID, rng); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + return qemuMonitorJSONAttachRNGDev(mon, chrID, objID, rng); +} + +int qemuMonitorDetachRNGDev(qemuMonitorPtr mon, + const char *objID) +{ + VIR_DEBUG("mon=%p objID=%s", mon, objID); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + return qemuMonitorDelObject(mon, objID); +} + + int qemuMonitorGetDeviceAliases(qemuMonitorPtr mon, char ***aliases) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index edab66f..99587e8 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -865,6 +865,13 @@ int qemuMonitorAttachCharDev(qemuMonitorPtr mon, int qemuMonitorDetachCharDev(qemuMonitorPtr mon, const char *chrID); +int qemuMonitorAttachRNGDev(qemuMonitorPtr mon, + const char *chrID, + const char *objID, + virDomainRNGDefPtr rng); +int qemuMonitorDetachRNGDev(qemuMonitorPtr mon, + const char *objID); + int qemuMonitorGetDeviceAliases(qemuMonitorPtr mon, char ***aliases); -- 1.8.3.1
On 01/03/15 06:06, Luyao Huang wrote:
These 2 functions just do some basic check and then call qemuMonitorJSONAttachRNGDev and qemuMonitorDelObject to help us.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_monitor.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 7 +++++++ 2 files changed, 50 insertions(+)
This patch can be folded into the previous one. And again the commit message could be improved. Peter
On 01/05/2015 11:31 PM, Peter Krempa wrote:
On 01/03/15 06:06, Luyao Huang wrote:
These 2 functions just do some basic check and then call qemuMonitorJSONAttachRNGDev and qemuMonitorDelObject to help us.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_monitor.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 7 +++++++ 2 files changed, 50 insertions(+)
This patch can be folded into the previous one. And again the commit message could be improved. Thanks for your review. Okay, i will merge them into patch "qemu: add a functions for attach a rng object in json monitor" Peter
Luyao
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_audit.c | 2 +- src/conf/domain_audit.h | 7 +++++++ src/libvirt_private.syms | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index fcf9df7..159ebf5 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -242,7 +242,7 @@ virDomainAuditDisk(virDomainObjPtr vm, } -static void +void virDomainAuditRNG(virDomainObjPtr vm, virDomainRNGDefPtr oldDef, virDomainRNGDefPtr newDef, const char *reason, bool success) diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h index 3434feb..4c1ef90 100644 --- a/src/conf/domain_audit.h +++ b/src/conf/domain_audit.h @@ -117,5 +117,12 @@ void virDomainAuditChardev(virDomainObjPtr vm, const char *reason, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +void virDomainAuditRNG(virDomainObjPtr vm, + virDomainRNGDefPtr oldDef, + virDomainRNGDefPtr newDef, + const char *reason, + bool success) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); + #endif /* __VIR_DOMAIN_AUDIT_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index deab4cf..9ed4583 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -123,6 +123,7 @@ virDomainAuditMemory; virDomainAuditNet; virDomainAuditNetDevice; virDomainAuditRedirdev; +virDomainAuditRNG; virDomainAuditSecurityLabel; virDomainAuditStart; virDomainAuditStop; -- 1.8.3.1
In subject: audit: export virDomainAuditRNG On 01/03/15 06:06, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_audit.c | 2 +- src/conf/domain_audit.h | 7 +++++++ src/libvirt_private.syms | 1 + 3 files changed, 9 insertions(+), 1 deletion(-)
ACK, Peter
On 01/05/2015 11:32 PM, Peter Krempa wrote:
In subject:
audit: export virDomainAuditRNG
On 01/03/15 06:06, Luyao Huang wrote:
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/conf/domain_audit.c | 2 +- src/conf/domain_audit.h | 7 +++++++ src/libvirt_private.syms | 1 + 3 files changed, 9 insertions(+), 1 deletion(-)
ACK, Thanks, i will update the subject in next version. Peter
Luyao
We have enough patches for hotplug RNG device, maybe we can implement live hotplug of a RNG device. Signed-off-by: Luyao Huang <lhuang@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_driver.c b/src/qemu/qemu_driver.c index 73a825d..2ad6e01 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6936,6 +6936,13 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, dev->data.chr = NULL; break; + case VIR_DOMAIN_DEVICE_RNG: + ret = qemuDomainAttachRNGDevice(driver, vm, + dev->data.rng); + if (!ret) + dev->data.rng = NULL; + break; + case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -6947,7 +6954,6 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: 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; + } + + 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) + return ret; + } + releaseaddr = true; + if (!(devstr = qemuBuildRNGDevStr(vmdef, rng, priv->qemuCaps))) + return ret; + + 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; + } 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, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 7b838ee..fbf009f 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -98,6 +98,9 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr); +int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng); int -- 1.8.3.1
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@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.
+ + 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.
+ 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.
+ + 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.
+ } 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
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@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
We have enough patches for hotunplug RNG device, maybe we can implement live hotunplug of a RNG device. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 4 +- 3 files changed, 102 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2ad6e01..f7600f3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7017,6 +7017,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_RNG: + ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng); + break; case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: @@ -7027,7 +7030,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7f1e612..d61e2a1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2921,6 +2921,52 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, return ret; } +static int +qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + virObjectEventPtr event; + char *charAlias = NULL; + char *objAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret = -1; + int rc; + + VIR_DEBUG("Removing RNG device %s from domain %p %s", + rng->info.alias, vm, vm->def->name); + + if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) + goto cleanup; + + if (virAsprintf(&charAlias, "char%s", rng->info.alias) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachRNGDev(priv->mon, objAlias); + if (rc == 0 && rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + qemuDomainObjExitMonitor(driver, vm); + + virDomainAuditRNG(vm, rng, NULL, "detach", rc == 0); + + if (rc < 0) + goto cleanup; + + event = virDomainEventDeviceRemovedNewFromObj(vm, rng->info.alias); + if (event) + qemuDomainEventQueue(driver, event); + + qemuDomainRNGRemove(vm->def, rng); + qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); + virDomainRNGDefFree(rng); + ret = 0; + + cleanup: + VIR_FREE(charAlias); + VIR_FREE(objAlias); + return ret; +} void qemuDomainRemoveDevice(virQEMUDriverPtr driver, @@ -2944,6 +2990,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_CHR: qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_RNG: + qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: @@ -2958,7 +3007,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: @@ -3830,3 +3878,50 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, VIR_FREE(devstr); return ret; } + +int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + virDomainRNGDefPtr tmpRNG; + int rc; + + if (!(tmpRNG = virDomainRNGFind(vmdef, rng))) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return ret; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return ret; + } + + if (!tmpRNG->info.alias && qemuAssignDeviceRNGAlias(vmdef, tmpRNG, -1) < 0) + return ret; + + sa_assert(tmpRNG->info.alias); + + qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info); + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDelDevice(priv->mon, tmpRNG->info.alias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG); + else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index fbf009f..4f7756b 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -101,7 +101,9 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng); - +int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng); int qemuDomainChrInsert(virDomainDefPtr vmdef, -- 1.8.3.1
On 01/03/15 06:06, Luyao Huang wrote:
We have enough patches for hotunplug RNG device, maybe we can implement live hotunplug of a RNG device.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 4 +- 3 files changed, 102 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2ad6e01..f7600f3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7017,6 +7017,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_RNG: + ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng); + break; case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: @@ -7027,7 +7030,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7f1e612..d61e2a1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2921,6 +2921,52 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, return ret; }
+static int +qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + virObjectEventPtr event; + char *charAlias = NULL; + char *objAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret = -1; + int rc; + + VIR_DEBUG("Removing RNG device %s from domain %p %s", + rng->info.alias, vm, vm->def->name); + + if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) + goto cleanup; + + if (virAsprintf(&charAlias, "char%s", rng->info.alias) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachRNGDev(priv->mon, objAlias); + if (rc == 0 && rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + qemuDomainObjExitMonitor(driver, vm); + + virDomainAuditRNG(vm, rng, NULL, "detach", rc == 0); + + if (rc < 0) + goto cleanup; + + event = virDomainEventDeviceRemovedNewFromObj(vm, rng->info.alias); + if (event) + qemuDomainEventQueue(driver, event); + + qemuDomainRNGRemove(vm->def, rng); + qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); + virDomainRNGDefFree(rng); + ret = 0; + + cleanup: + VIR_FREE(charAlias); + VIR_FREE(objAlias); + return ret; +}
void qemuDomainRemoveDevice(virQEMUDriverPtr driver, @@ -2944,6 +2990,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_CHR: qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_RNG: + qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); + break;
case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: @@ -2958,7 +3007,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: @@ -3830,3 +3878,50 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, VIR_FREE(devstr); return ret; } + +int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + virDomainRNGDefPtr tmpRNG; + int rc; + + if (!(tmpRNG = virDomainRNGFind(vmdef, rng))) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return ret; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return ret; + } + + if (!tmpRNG->info.alias && qemuAssignDeviceRNGAlias(vmdef, tmpRNG, -1) < 0) + return ret; + + sa_assert(tmpRNG->info.alias);
Did coverity complain here?
+ + qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info); + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDelDevice(priv->mon, tmpRNG->info.alias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG); + else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index fbf009f..4f7756b 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -101,7 +101,9 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng); - +int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng);
int qemuDomainChrInsert(virDomainDefPtr vmdef,
Looks good for now. Peter
On 01/03/15 06:06, Luyao Huang wrote:
We have enough patches for hotunplug RNG device, maybe we can implement live hotunplug of a RNG device.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 4 +- 3 files changed, 102 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2ad6e01..f7600f3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7017,6 +7017,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_RNG: + ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng); + break; case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: @@ -7027,7 +7030,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7f1e612..d61e2a1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2921,6 +2921,52 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, return ret; }
+static int +qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + virObjectEventPtr event; + char *charAlias = NULL; + char *objAlias = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret = -1; + int rc; + + VIR_DEBUG("Removing RNG device %s from domain %p %s", + rng->info.alias, vm, vm->def->name); + + if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) + goto cleanup; + + if (virAsprintf(&charAlias, "char%s", rng->info.alias) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachRNGDev(priv->mon, objAlias); + if (rc == 0 && rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) + ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + qemuDomainObjExitMonitor(driver, vm); + + virDomainAuditRNG(vm, rng, NULL, "detach", rc == 0); + + if (rc < 0) + goto cleanup; + + event = virDomainEventDeviceRemovedNewFromObj(vm, rng->info.alias); + if (event) + qemuDomainEventQueue(driver, event); + + qemuDomainRNGRemove(vm->def, rng); + qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); + virDomainRNGDefFree(rng); + ret = 0; + + cleanup: + VIR_FREE(charAlias); + VIR_FREE(objAlias); + return ret; +}
void qemuDomainRemoveDevice(virQEMUDriverPtr driver, @@ -2944,6 +2990,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_CHR: qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); break; + case VIR_DOMAIN_DEVICE_RNG: + qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); + break;
case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: @@ -2958,7 +3007,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: @@ -3830,3 +3878,50 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, VIR_FREE(devstr); return ret; } + +int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + virDomainRNGDefPtr tmpRNG; + int rc; + + if (!(tmpRNG = virDomainRNGFind(vmdef, rng))) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return ret; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return ret; + } + + if (!tmpRNG->info.alias && qemuAssignDeviceRNGAlias(vmdef, tmpRNG, -1) < 0) + return ret; + + sa_assert(tmpRNG->info.alias); Did coverity complain here? I guess it will, this place just like the scene in commit e7e05801. but i am not sure (i don't use coverity because the resource is very
On 01/05/2015 11:54 PM, Peter Krempa wrote: limit), maybe we can remove it first, then wait for coverity test result.
+ + qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info); + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDelDevice(priv->mon, tmpRNG->info.alias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG); + else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index fbf009f..4f7756b 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -101,7 +101,9 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng); - +int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr rng);
int qemuDomainChrInsert(virDomainDefPtr vmdef,
Looks good for now.
Peter
Luyao
participants (4)
-
Eric Blake -
lhuang -
Luyao Huang
-
Peter Krempa