5:05 a.m.
Pavel Hrdina (2):
qemu_driver: live/config checks cleanup
qemu_driver: remove duplicate code
src/qemu/qemu_driver.c | 85 ++++++--------------------------------------------
1 file changed, 9 insertions(+), 76 deletions(-)
--
2.4.5
5:05 a.m.
New subject: [libvirt] [PATCH 1/2] qemu_driver: live/config checks cleanup
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_driver.c | 79 ++++++--------------------------------------------
1 file changed, 9 insertions(+), 70 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7a04e67..4cfae03 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8409,7 +8409,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char
*xml,
virDomainDefPtr vmdef = NULL;
virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
int ret = -1;
- unsigned int affect, parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+ unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
virQEMUCapsPtr qemuCaps = NULL;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = NULL;
@@ -8422,8 +8422,6 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char
*xml,
cfg = virQEMUDriverGetConfig(driver);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
-
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8438,26 +8436,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char
*xml,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
- if (virDomainObjIsActive(vm)) {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
- } else {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_CONFIG;
- /* check consistency between flags and the vm state */
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot do live update a device on "
- "inactive domain"));
- goto endjob;
- }
- }
-
- if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot modify device on transient domain"));
- goto endjob;
- }
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
caps, driver->xmlopt,
@@ -8556,7 +8536,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
int ret = -1;
- unsigned int affect;
virQEMUCapsPtr qemuCaps = NULL;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = NULL;
@@ -8570,8 +8549,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
cfg = virQEMUDriverGetConfig(driver);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
-
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8586,33 +8563,15 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
- if (virDomainObjIsActive(vm)) {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
- } else {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_CONFIG;
- /* check consistency between flags and the vm state */
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot do live update a device on "
- "inactive domain"));
- goto endjob;
- }
- }
-
- if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot modify device on transient domain"));
- goto endjob;
- }
-
dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
caps, driver->xmlopt,
VIR_DOMAIN_DEF_PARSE_INACTIVE);
if (dev == NULL)
goto endjob;
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
+
if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
flags & VIR_DOMAIN_AFFECT_LIVE) {
/* If we are affecting both CONFIG and LIVE
@@ -8695,7 +8654,7 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char
*xml,
virDomainDefPtr vmdef = NULL;
virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
int ret = -1;
- unsigned int affect, parse_flags = 0;
+ unsigned int parse_flags = 0;
virQEMUCapsPtr qemuCaps = NULL;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = NULL;
@@ -8706,8 +8665,6 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char
*xml,
cfg = virQEMUDriverGetConfig(driver);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
-
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8722,26 +8679,8 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char
*xml,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
- if (virDomainObjIsActive(vm)) {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
- } else {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_CONFIG;
- /* check consistency between flags and the vm state */
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot do live update a device on "
- "inactive domain"));
- goto endjob;
- }
- }
-
- if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot modify device on transient domain"));
- goto endjob;
- }
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
if ((flags & VIR_DOMAIN_AFFECT_CONFIG) &&
!(flags & VIR_DOMAIN_AFFECT_LIVE))
--
2.4.5
3:56 a.m.
New subject: [libvirt] [PATCH 1/2] qemu_driver: live/config checks cleanup
On 01/07/15 12:05, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_driver.c | 79 ++++++--------------------------------------------
1 file changed, 9 insertions(+), 70 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7a04e67..4cfae03 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8409,7 +8409,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char
*xml,
virDomainDefPtr vmdef = NULL;
virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
int ret = -1;
- unsigned int affect, parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+ unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
virQEMUCapsPtr qemuCaps = NULL;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = NULL;
@@ -8422,8 +8422,6 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char
*xml,
cfg = virQEMUDriverGetConfig(driver);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
-
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8438,26 +8436,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const
char *xml,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
- if (virDomainObjIsActive(vm)) {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
- } else {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_CONFIG;
- /* check consistency between flags and the vm state */
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot do live update a device on "
- "inactive domain"));
- goto endjob;
- }
- }
-
- if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot modify device on transient domain"));
- goto endjob;
- }
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
caps, driver->xmlopt,
@@ -8556,7 +8536,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
int ret = -1;
- unsigned int affect;
virQEMUCapsPtr qemuCaps = NULL;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = NULL;
@@ -8570,8 +8549,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
cfg = virQEMUDriverGetConfig(driver);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
-
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8586,33 +8563,15 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
- if (virDomainObjIsActive(vm)) {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
- } else {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_CONFIG;
- /* check consistency between flags and the vm state */
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot do live update a device on "
- "inactive domain"));
- goto endjob;
- }
- }
-
- if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot modify device on transient domain"));
- goto endjob;
- }
-
dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
caps, driver->xmlopt,
VIR_DOMAIN_DEF_PARSE_INACTIVE);
if (dev == NULL)
goto endjob;
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
+
if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
flags & VIR_DOMAIN_AFFECT_LIVE) {
/* If we are affecting both CONFIG and LIVE
@@ -8695,7 +8654,7 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char
*xml,
virDomainDefPtr vmdef = NULL;
virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
int ret = -1;
- unsigned int affect, parse_flags = 0;
+ unsigned int parse_flags = 0;
virQEMUCapsPtr qemuCaps = NULL;
qemuDomainObjPrivatePtr priv;
virQEMUDriverConfigPtr cfg = NULL;
@@ -8706,8 +8665,6 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char
*xml,
cfg = virQEMUDriverGetConfig(driver);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG);
-
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8722,26 +8679,8 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const
char *xml,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
- if (virDomainObjIsActive(vm)) {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
- } else {
- if (affect == VIR_DOMAIN_AFFECT_CURRENT)
- flags |= VIR_DOMAIN_AFFECT_CONFIG;
- /* check consistency between flags and the vm state */
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot do live update a device on "
- "inactive domain"));
- goto endjob;
- }
- }
-
- if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot modify device on transient domain"));
- goto endjob;
- }
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
if ((flags & VIR_DOMAIN_AFFECT_CONFIG) &&
!(flags & VIR_DOMAIN_AFFECT_LIVE))
ACK.
Erik
5:05 a.m.
New subject: [libvirt] [PATCH 2/2] qemu_driver: remove duplicate code
The copy of persistent definition is already done in
virDomainLiveConfigHelperMethod few lines above.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4cfae03..ca93a1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10336,12 +10336,6 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
&vmdef) < 0)
goto endjob;
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
- /* Make a copy for updated domain. */
- if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt)))
- goto endjob;
- }
-
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
virReportError(VIR_ERR_OPERATION_INVALID,
--
2.4.5
7:01 a.m.
New subject: [libvirt] [PATCH 2/2] qemu_driver: remove duplicate code
On 01/07/15 12:05, Pavel Hrdina wrote:
The copy of persistent definition is already done in
virDomainLiveConfigHelperMethod few lines above.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4cfae03..ca93a1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10336,12 +10336,6 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
&vmdef) < 0)
goto endjob;
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
- /* Make a copy for updated domain. */
- if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt)))
- goto endjob;
- }
-
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
virReportError(VIR_ERR_OPERATION_INVALID,
NACK. It's not true, that a copy is made in
virDomainLiveConfigHelperMethod, only a reference to domain definition
is returned. The problem is that we free vmdef at the end of the API
which might not result in a desired behavior, let's consider an inactive
persistent domain and you try to set one of scheduler params, instead of
modifying an freeing a copy, you manipulate and free the original
instance, what happens then? Yep, daemon crashes in the next API (OK,
not every time, it requires a bit of luck but after a couple of minutes
I managed to do that as well).
Erik
8:28 a.m.
New subject: [libvirt] [PATCH 2/2] qemu_driver: remove duplicate code
On Fri, Jul 03, 2015 at 02:01:46PM +0200, Erik Skultety wrote:
On 01/07/15 12:05, Pavel Hrdina wrote:
> The copy of persistent definition is already done in
> virDomainLiveConfigHelperMethod few lines above.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4cfae03..ca93a1a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10336,12 +10336,6 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
> &vmdef) < 0)
> goto endjob;
>
> - if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> - /* Make a copy for updated domain. */
> - if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt)))
> - goto endjob;
> - }
> -
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
>
NACK. It's not true, that a copy is made in
virDomainLiveConfigHelperMethod, only a reference to domain definition
is returned. The problem is that we free vmdef at the end of the API
which might not result in a desired behavior, let's consider an inactive
persistent domain and you try to set one of scheduler params, instead of
modifying an freeing a copy, you manipulate and free the original
instance, what happens then? Yep, daemon crashes in the next API (OK,
not every time, it requires a bit of luck but after a couple of minutes
I managed to do that as well).
True, I've missed that in the helper we don't create a copy, just take a
reference. This makes perfect sense and in fact it's an exception, where we use
a copy to update the domain and in case of failure just drop the copy and leave
the domain definition intact. I thought, that there is no API that have this
behavior.
I'll push only the first patch, thanks for review.
Pavel
Erik
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3430
days inactive
3432
days old
5 comments
2 participants
participants (2)
-
Erik Skultety -
Pavel Hrdina