On 01/07/15 12:05, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina <phrdina@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

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@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