[libvirt] [PATCH v2 0/4] cleanup resolving persistent/running/current flags
Original name of series was: Subject: [PATCH 0/3] make virDomainObjGetPersistentDef return only persistent definition Changes from version1 ===================== Original motivation of the series was fixing virDomainObjGetPersistentDef so that it would not return configs other than persistent. However the final patch that does it in the first version is absent in current version. I think the fix should be done another way and in different series. Thus this version has only misc cleanups which are better splitted into patches in this version. A few words why I leave patch for virDomainObjGetPersistentDef. First I should not return NULL from the function as this value already has different meaning - memory allocation error and there is code that checks for this error. This code calls virDomainObjGetPersistentDef unconditionally but later use the return value only for persistent domains, thus it could get NULL pointer on function call and treat it as an allocation error. I think the proper way would be chaning virDomainObjGetPersistentDef so it could not fail that is not call virDomainObjSetDefTransient. Looks like this is already true because drivers that distiguish between running/persistent config call virDomainObjSetDefTransient ealy in the domain start process (and even two times) so that later calls for virDomainObjSetDefTransient are unnecessary. Nikolay Shirokovskiy (4): virDomainObjUpdateModificationImpact: reduce nesting libxlDomainSetMemoryFlags : reuse virDomainLiveConfigHelperMethod lxc, libxl: reuse virDomainObjUpdateModificationImpact libxlDomainPinVcpuFlags: remove check duplicates src/conf/domain_conf.c | 12 +++--- src/libxl/libxl_driver.c | 103 ++++------------------------------------------- src/lxc/lxc_driver.c | 75 +++------------------------------- 3 files changed, 19 insertions(+), 171 deletions(-) -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0d2957b..d6c33f6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2872,13 +2872,11 @@ virDomainObjUpdateModificationImpact(virDomainObjPtr vm, return -1; } - if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("transient domains do not have any " - "persistent config")); - return -1; - } + if (!vm->persistent && (*flags & VIR_DOMAIN_AFFECT_CONFIG)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domains do not have any " + "persistent config")); + return -1; } return 0; -- 1.8.3.1
Please, disregard this. It's version of the other 1/4 patch which differs only in commit message. On 24.02.2016 11:38, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0d2957b..d6c33f6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2872,13 +2872,11 @@ virDomainObjUpdateModificationImpact(virDomainObjPtr vm, return -1; }
- if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("transient domains do not have any " - "persistent config")); - return -1; - } + if (!vm->persistent && (*flags & VIR_DOMAIN_AFFECT_CONFIG)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domains do not have any " + "persistent config")); + return -1; }
return 0;
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0d2957b..d6c33f6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2872,13 +2872,11 @@ virDomainObjUpdateModificationImpact(virDomainObjPtr vm, return -1; } - if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("transient domains do not have any " - "persistent config")); - return -1; - } + if (!vm->persistent && (*flags & VIR_DOMAIN_AFFECT_CONFIG)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domains do not have any " + "persistent config")); + return -1; } return 0; -- 1.8.3.1
On 02/24/2016 03:38 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
ACK - I've adjusted the commit message mainly to give some more context and pushed this patch. John
Flag expansion is the same as in virDomainObjUpdateModificationImpact which virDomainLiveConfigHelperMethod calls internally. The difference is merely in implementation. Original code don't use mask and handle case when VIR_DOMAIN_MEM_MAXIMUM is set explicitly. Other parts are no different. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 34 ++-------------------------------- 1 file changed, 2 insertions(+), 32 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 404016e..06feea5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1444,7 +1444,6 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm; virDomainDefPtr persistentDef = NULL; - bool isActive; int ret = -1; virCheckFlags(VIR_DOMAIN_MEM_LIVE | @@ -1460,38 +1459,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_MEM_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_MEM_LIVE; - else - flags = VIR_DOMAIN_MEM_CONFIG; - } - if (flags == VIR_DOMAIN_MEM_MAXIMUM) { - if (isActive) - flags = VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_MAXIMUM; - else - flags = VIR_DOMAIN_MEM_CONFIG | VIR_DOMAIN_MEM_MAXIMUM; - } - - if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot set memory on an inactive domain")); + if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) goto endjob; - } - - if (flags & VIR_DOMAIN_MEM_CONFIG) { - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto endjob; - } - if (!(persistentDef = virDomainObjGetPersistentDef(cfg->caps, - driver->xmlopt, - vm))) - goto endjob; - } if (flags & VIR_DOMAIN_MEM_MAXIMUM) { /* resize the maximum memory */ -- 1.8.3.1
$subj: Use the "libxl: ..." prefix please That way someone filtering on libxl patches may pick up on this. I can fix this for this one... On 02/24/2016 03:38 AM, Nikolay Shirokovskiy wrote:
Flag expansion is the same as in virDomainObjUpdateModificationImpact which virDomainLiveConfigHelperMethod calls internally. The difference is merely in implementation. Original code don't use mask and handle case when VIR_DOMAIN_MEM_MAXIMUM is set explicitly.
Other parts are no different.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 34 ++-------------------------------- 1 file changed, 2 insertions(+), 32 deletions(-)
I think what's being done is OK, but just to be save, I've CC'd Jim Fehlig who typically reviews libxl patches just to be sure before I push the change. John
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 404016e..06feea5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1444,7 +1444,6 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm; virDomainDefPtr persistentDef = NULL; - bool isActive; int ret = -1;
virCheckFlags(VIR_DOMAIN_MEM_LIVE | @@ -1460,38 +1459,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup;
- isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_MEM_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_MEM_LIVE; - else - flags = VIR_DOMAIN_MEM_CONFIG; - } - if (flags == VIR_DOMAIN_MEM_MAXIMUM) { - if (isActive) - flags = VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_MAXIMUM; - else - flags = VIR_DOMAIN_MEM_CONFIG | VIR_DOMAIN_MEM_MAXIMUM; - } - - if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot set memory on an inactive domain")); + if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) goto endjob; - } - - if (flags & VIR_DOMAIN_MEM_CONFIG) { - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto endjob; - } - if (!(persistentDef = virDomainObjGetPersistentDef(cfg->caps, - driver->xmlopt, - vm))) - goto endjob; - }
if (flags & VIR_DOMAIN_MEM_MAXIMUM) { /* resize the maximum memory */
On 03/01/2016 08:46 AM, John Ferlan wrote:
$subj:
Use the "libxl: ..." prefix please
That way someone filtering on libxl patches may pick up on this. I can fix this for this one...
On 02/24/2016 03:38 AM, Nikolay Shirokovskiy wrote:
Flag expansion is the same as in virDomainObjUpdateModificationImpact which virDomainLiveConfigHelperMethod calls internally. The difference is merely in implementation. Original code don't use mask and handle case when VIR_DOMAIN_MEM_MAXIMUM is set explicitly.
Other parts are no different.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 34 ++-------------------------------- 1 file changed, 2 insertions(+), 32 deletions(-)
I think what's being done is OK, but just to be save, I've CC'd Jim Fehlig who typically reviews libxl patches just to be sure before I push the change.
The change looks good to me. In fact, it seems to be an improvement over the existing logic. ACK. Regards, Jim
John
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 404016e..06feea5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1444,7 +1444,6 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm; virDomainDefPtr persistentDef = NULL; - bool isActive; int ret = -1;
virCheckFlags(VIR_DOMAIN_MEM_LIVE | @@ -1460,38 +1459,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup;
- isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_MEM_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_MEM_LIVE; - else - flags = VIR_DOMAIN_MEM_CONFIG; - } - if (flags == VIR_DOMAIN_MEM_MAXIMUM) { - if (isActive) - flags = VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_MAXIMUM; - else - flags = VIR_DOMAIN_MEM_CONFIG | VIR_DOMAIN_MEM_MAXIMUM; - } - - if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot set memory on an inactive domain")); + if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) goto endjob; - } - - if (flags & VIR_DOMAIN_MEM_CONFIG) { - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto endjob; - } - if (!(persistentDef = virDomainObjGetPersistentDef(cfg->caps, - driver->xmlopt, - vm))) - goto endjob; - }
if (flags & VIR_DOMAIN_MEM_MAXIMUM) { /* resize the maximum memory */
First fortunately all versions of *_CURRENT, *_CONFIG and *_LIVE flags are numerically equal. Second libxl functions that don't use masks for flag expansion forbid extra flags via virCheckFlags before. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 63 ++++------------------------------------ src/lxc/lxc_driver.c | 75 ++++-------------------------------------------- 2 files changed, 12 insertions(+), 126 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 06feea5..7e65ba2 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3660,25 +3660,8 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto endjob; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_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_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, @@ -3768,25 +3751,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto endjob; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_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_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, @@ -3873,25 +3839,8 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto cleanup; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 41639ac..316c60c 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4993,43 +4993,22 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 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 cleanup; - } - } - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; - if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, @@ -5121,41 +5100,20 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); - affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 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 cleanup; - } - } - - if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -5235,40 +5193,19 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 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 cleanup; - } - } - - if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; -- 1.8.3.1
On 02/24/2016 03:38 AM, Nikolay Shirokovskiy wrote:
First fortunately all versions of *_CURRENT, *_CONFIG and *_LIVE flags are numerically equal. Second libxl functions that don't use masks for flag expansion forbid extra flags via virCheckFlags before.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 63 ++++------------------------------------ src/lxc/lxc_driver.c | 75 ++++-------------------------------------------- 2 files changed, 12 insertions(+), 126 deletions(-)
This should be two separate patches and use of singular libxl: and lxc: prefixes on the subject line. #1 For libxl driver changes (there's a bug there too) #2 for lxc changes This one I'll ask you to fix the bug and repost as two separate patches
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 06feea5..7e65ba2 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3660,25 +3660,8 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup;
- if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto endjob; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_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_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, @@ -3768,25 +3751,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup;
- if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto endjob; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_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_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, @@ -3873,25 +3839,8 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto cleanup; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob;
This fails to compile since endjob doesn't exist in this context. John
if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 41639ac..316c60c 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4993,43 +4993,22 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup;
if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 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 cleanup; - } - } - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup;
- if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup;
dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, @@ -5121,41 +5100,20 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup;
if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 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 cleanup; - } - } - - if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup;
if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -5235,40 +5193,19 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1);
- affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup;
if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 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 cleanup; - } - } - - if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup;
if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup;
On 01.03.2016 18:46, John Ferlan wrote:
On 02/24/2016 03:38 AM, Nikolay Shirokovskiy wrote:
First fortunately all versions of *_CURRENT, *_CONFIG and *_LIVE flags are numerically equal. Second libxl functions that don't use masks for flag expansion forbid extra flags via virCheckFlags before.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 63 ++++------------------------------------ src/lxc/lxc_driver.c | 75 ++++-------------------------------------------- 2 files changed, 12 insertions(+), 126 deletions(-)
This should be two separate patches and use of singular libxl: and lxc: prefixes on the subject line.
#1 For libxl driver changes (there's a bug there too) #2 for lxc changes
This one I'll ask you to fix the bug and repost as two separate patches
OK. I'll resend soon.
virDomainLiveConfigHelperMethod function checks this condition already. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7e65ba2..1428239 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2315,12 +2315,6 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && !virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is inactive")); - goto endjob; - } - if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, &targetDef) < 0) goto endjob; -- 1.8.3.1
On 02/24/2016 03:38 AM, Nikolay Shirokovskiy wrote:
virDomainLiveConfigHelperMethod function checks this condition already.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 6 ------ 1 file changed, 6 deletions(-)
ACK - I've adjusted the commit message and pushed this one. John
virDomainLiveConfigHelperMethod function checks this condition already. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7e65ba2..1428239 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2315,12 +2315,6 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && !virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is inactive")); - goto endjob; - } - if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, &targetDef) < 0) goto endjob; -- 1.8.3.1
Please, disregard this. It's version of the other 1/4 patch which differs only in commit message. On 24.02.2016 11:38, Nikolay Shirokovskiy wrote:
virDomainLiveConfigHelperMethod function checks this condition already.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libxl/libxl_driver.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7e65ba2..1428239 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2315,12 +2315,6 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup;
- if ((flags & VIR_DOMAIN_AFFECT_LIVE) && !virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is inactive")); - goto endjob; - } - if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, &targetDef) < 0) goto endjob;
participants (3)
-
Jim Fehlig -
John Ferlan -
Nikolay Shirokovskiy