[libvirt] [PATCH 0/3] make virDomainObjGetPersistentDef return only persistent definition
I want to make virDomainObjGetPersistentDef return only persistent definition as one can infer from its name. Patches 1 and 2 clean things up to make reasoning that this operation is possible more clear. Nikolay Shirokovskiy (3): domain: reuse update flags checking functions libxl: remove check duplicates common-impl: make virDomainObjGetPersistentDef return only persistent config src/conf/domain_conf.c | 19 ++++----- src/libxl/libxl_driver.c | 103 ++++------------------------------------------- src/lxc/lxc_driver.c | 75 +++------------------------------- 3 files changed, 24 insertions(+), 173 deletions(-) -- 1.8.3.1
Uses virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact appropriately. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++--- src/libxl/libxl_driver.c | 97 ++++-------------------------------------------- src/lxc/lxc_driver.c | 75 +++---------------------------------- 3 files changed, 19 insertions(+), 165 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9706b0..e54c097 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2880,13 +2880,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; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d4e9c2a7..508bae4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1440,7 +1440,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 | @@ -1456,38 +1455,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 */ @@ -3680,25 +3650,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, @@ -3788,25 +3741,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, @@ -3893,25 +3829,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 24b9622..aea39c1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4989,43 +4989,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, @@ -5117,41 +5096,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; @@ -5231,40 +5189,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 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
Uses virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact appropriately.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++--- src/libxl/libxl_driver.c | 97 ++++-------------------------------------------- src/lxc/lxc_driver.c | 75 +++---------------------------------- 3 files changed, 19 insertions(+), 165 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9706b0..e54c097 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2880,13 +2880,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)) {
Not the same check. A 'transient' domain is running, but has no on disk config. So if some command (e.g. virsh $dom setmem 20G --config) is issued, we want to stop that from happening on a transient domain. However, there may be other commands executed on a transient domain that we want to allow to happen, thus we cannot change this into an && check. It needs to be "if attempting to affect config", then if not persistent, then error [else allow the change to the config]. The rest is libxl specific and while it seems reasonable, I didn't check each change... I did note there is at least one change which has command specific logic dealing with flags adjustments not related to active, live, persistent, config, current, etc. removed which doesn't seem like it's right... John
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domains do not have any " + "persistent config")); + return -1; }
return 0; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d4e9c2a7..508bae4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1440,7 +1440,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 | @@ -1456,38 +1455,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; - }
VIR_DOMAIN_MEM_MAXIMUM has nothing to do with CONFIG, LIVE, CURRENT...
- - 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 */ @@ -3680,25 +3650,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, @@ -3788,25 +3741,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, @@ -3893,25 +3829,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 24b9622..aea39c1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4989,43 +4989,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, @@ -5117,41 +5096,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; @@ -5231,40 +5189,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 02.02.2016 01:48, John Ferlan wrote:
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
Uses virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact appropriately.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++--- src/libxl/libxl_driver.c | 97 ++++-------------------------------------------- src/lxc/lxc_driver.c | 75 +++---------------------------------- 3 files changed, 19 insertions(+), 165 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9706b0..e54c097 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2880,13 +2880,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)) {
Not the same check.
A 'transient' domain is running, but has no on disk config.
So if some command (e.g. virsh $dom setmem 20G --config) is issued, we want to stop that from happening on a transient domain. However, there may be other commands executed on a transient domain that we want to allow to happen, thus we cannot change this into an && check. It needs to be "if attempting to affect config", then if not persistent, then error [else allow the change to the config].
Well it is not principal to me. I thought as new and old logically equivalent we can get rid of extra nesting. (By the way we can exchage operands of && as they don't influence each other.)
The rest is libxl specific and while it seems reasonable, I didn't check each change... I did note there is at least one change which has command specific logic dealing with flags adjustments not related to active, live, persistent, config, current, etc. removed which doesn't seem like it's right...
that place needs extra explanations, see below
John
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domains do not have any " + "persistent config")); + return -1; }
return 0; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d4e9c2a7..508bae4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1440,7 +1440,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 | @@ -1456,38 +1455,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; - }
VIR_DOMAIN_MEM_MAXIMUM has nothing to do with CONFIG, LIVE, CURRENT...
This place is strange but correct at least until no extra memory flags introduced. Basically these two if blocks resolve 'current' flag but instead of checking flags against 'live & config' mask as in virDomainObjUpdateModificationImpact the resolving is expanded into two blocks.
On 02/02/2016 08:04 AM, Nikolay Shirokovskiy wrote:
On 02.02.2016 01:48, John Ferlan wrote:
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
Uses virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact appropriately.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++--- src/libxl/libxl_driver.c | 97 ++++-------------------------------------------- src/lxc/lxc_driver.c | 75 +++---------------------------------- 3 files changed, 19 insertions(+), 165 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9706b0..e54c097 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2880,13 +2880,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)) {
Not the same check.
A 'transient' domain is running, but has no on disk config.
So if some command (e.g. virsh $dom setmem 20G --config) is issued, we want to stop that from happening on a transient domain. However, there may be other commands executed on a transient domain that we want to allow to happen, thus we cannot change this into an && check. It needs to be "if attempting to affect config", then if not persistent, then error [else allow the change to the config].
Well it is not principal to me. I thought as new and old logically equivalent we can get rid of extra nesting. (By the way we can exchage operands of && as they don't influence each other.)
Sorry for the delay, but other things were more important to do in the highly preemptible work queue. First off - please try to add a blank line around your responses - it's a readability thing... w/r/t this change - I'm probably over-thinking it; however, it's a concept and area of the code which is tricky and sensitive. There were also 3 different "things" happening here - let's keep them singular. Looking at the history, I see commit id '3d021381' split up the virDomainLiveConfigHelperMethod to be two functions, with the second being virDomainObjUpdateModificationImpact. Prior to that the "if (*flags & VIR_DOMAIN_AFFECT_CONFIG) {" had two sub-checks, the second of which is what's primarily left in virDomainLiveConfigHelperMethod. Anyway since this change is "standalone", please separate it from the rest... That is the domain_conf.c change should be it's own patch.
The rest is libxl specific and while it seems reasonable, I didn't check each change... I did note there is at least one change which has command specific logic dealing with flags adjustments not related to active, live, persistent, config, current, etc. removed which doesn't seem like it's right...
that place needs extra explanations, see below
John
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domains do not have any " + "persistent config")); + return -1; }
return 0; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d4e9c2a7..508bae4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1440,7 +1440,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 | @@ -1456,38 +1455,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; - }
VIR_DOMAIN_MEM_MAXIMUM has nothing to do with CONFIG, LIVE, CURRENT...
This place is strange but correct at least until no extra memory flags introduced. Basically these two if blocks resolve 'current' flag but instead of checking flags against 'live & config' mask as in virDomainObjUpdateModificationImpact the resolving is expanded into two blocks.
OK since it's the only path to call virDomainLiveConfigHelperMethod, let's make it a separate patch to make it more obvious (add something to the commit message, too) That leaves the others calling virDomainObjUpdateModificationImpact which should be a separate patch too. Hope this makes sense, John
On 19.02.2016 01:43, John Ferlan wrote:
On 02/02/2016 08:04 AM, Nikolay Shirokovskiy wrote:
On 02.02.2016 01:48, John Ferlan wrote:
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
Uses virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact appropriately.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 12 +++--- src/libxl/libxl_driver.c | 97 ++++-------------------------------------------- src/lxc/lxc_driver.c | 75 +++---------------------------------- 3 files changed, 19 insertions(+), 165 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9706b0..e54c097 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2880,13 +2880,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)) {
Not the same check.
A 'transient' domain is running, but has no on disk config.
So if some command (e.g. virsh $dom setmem 20G --config) is issued, we want to stop that from happening on a transient domain. However, there may be other commands executed on a transient domain that we want to allow to happen, thus we cannot change this into an && check. It needs to be "if attempting to affect config", then if not persistent, then error [else allow the change to the config].
Well it is not principal to me. I thought as new and old logically equivalent we can get rid of extra nesting. (By the way we can exchage operands of && as they don't influence each other.)
Sorry for the delay, but other things were more important to do in the highly preemptible work queue.
First off - please try to add a blank line around your responses - it's a readability thing...
Thank you for pointing this out. Somehow I failed to realize it and started to appreciate this rule only recently )
w/r/t this change - I'm probably over-thinking it; however, it's a concept and area of the code which is tricky and sensitive. There were also 3 different "things" happening here - let's keep them singular.
Looking at the history, I see commit id '3d021381' split up the virDomainLiveConfigHelperMethod to be two functions, with the second being virDomainObjUpdateModificationImpact. Prior to that the "if (*flags & VIR_DOMAIN_AFFECT_CONFIG) {" had two sub-checks, the second of which is what's primarily left in virDomainLiveConfigHelperMethod.
Anyway since this change is "standalone", please separate it from the rest... That is the domain_conf.c change should be it's own patch.
Ok.
The rest is libxl specific and while it seems reasonable, I didn't check each change... I did note there is at least one change which has command specific logic dealing with flags adjustments not related to active, live, persistent, config, current, etc. removed which doesn't seem like it's right...
that place needs extra explanations, see below
John
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domains do not have any " + "persistent config")); + return -1; }
return 0; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d4e9c2a7..508bae4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1440,7 +1440,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 | @@ -1456,38 +1455,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; - }
VIR_DOMAIN_MEM_MAXIMUM has nothing to do with CONFIG, LIVE, CURRENT...
This place is strange but correct at least until no extra memory flags introduced. Basically these two if blocks resolve 'current' flag but instead of checking flags against 'live & config' mask as in virDomainObjUpdateModificationImpact the resolving is expanded into two blocks.
OK since it's the only path to call virDomainLiveConfigHelperMethod, let's make it a separate patch to make it more obvious (add something to the commit message, too)
That leaves the others calling virDomainObjUpdateModificationImpact which should be a separate patch too.
Hope this makes sense,
Sure, this change deserves its own commit with all explanations in commit message.
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 508bae4..7b5c7bc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2306,12 +2306,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 01/15/2016 09:05 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(-)
This one seems reasonable and true. John
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 508bae4..7b5c7bc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2306,12 +2306,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;
It's rather unexpected that function with such a name could return transient definition. This is possible if we call it for a transient active domain. But if it is called in such a conditions? No. So we would better fix this case to return NULL. Calling conditions investigation. There are 4 distinct callers: 1. migration - explicitly set 'persistent' before calling. 2. libxl_driver First it assures that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. Then all usages of definition is under VIR_DOMAIN_VCPU_CONFIG flag, so 'persistent' is set. 3. virDomainObjCopyPersistentDef - all callers of this function has next structure: 1 call virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact thus we are sure that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. 2. call function of interest under VIR_DOMAIN_VCPU_CONFIG, so 'persistent' flag is set. 4. virDomainLiveConfigHelperMethod - the same reason as in 3. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e54c097..018c77e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2825,18 +2825,21 @@ virDomainObjSetDefTransient(virCapsPtr caps, /* * Return the persistent domain configuration. If domain is transient, - * return the running config. + * return NULL. * * @param caps pointer to capabilities info * @param xmlopt pointer to XML parser configuration object * @param domain domain object pointer - * @return NULL on error, virDOmainDefPtr on success + * @return NULL on error or transient domains, virDOmainDefPtr on success */ virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain) { + if (!domain->persistent) + return NULL; + if (virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) return NULL; -- 1.8.3.1
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
It's rather unexpected that function with such a name could return transient definition. This is possible if we call it for a transient active domain. But if it is called in such a conditions? No. So we would better fix this case to return NULL.
Calling conditions investigation.
There are 4 distinct callers:
1. migration - explicitly set 'persistent' before calling. 2. libxl_driver First it assures that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. Then all usages of definition is under VIR_DOMAIN_VCPU_CONFIG flag, so 'persistent' is set.
3. virDomainObjCopyPersistentDef - all callers of this function has next structure: 1 call virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact thus we are sure that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. 2. call function of interest under VIR_DOMAIN_VCPU_CONFIG, so 'persistent' flag is set.
4. virDomainLiveConfigHelperMethod - the same reason as in 3.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
You've based your assumptions off of the change you made in 1/3 to virDomainObjUpdateModificationImpact regarding combining that if statement. Also even if the following would fail, you would have need to provide some sort of virReportError. John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e54c097..018c77e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2825,18 +2825,21 @@ virDomainObjSetDefTransient(virCapsPtr caps,
/* * Return the persistent domain configuration. If domain is transient, - * return the running config. + * return NULL. * * @param caps pointer to capabilities info * @param xmlopt pointer to XML parser configuration object * @param domain domain object pointer - * @return NULL on error, virDOmainDefPtr on success + * @return NULL on error or transient domains, virDOmainDefPtr on success */ virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain) { + if (!domain->persistent) + return NULL; + if (virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) return NULL;
On 02.02.2016 15:13, John Ferlan wrote:
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
It's rather unexpected that function with such a name could return transient definition. This is possible if we call it for a transient active domain. But if it is called in such a conditions? No. So we would better fix this case to return NULL.
Calling conditions investigation.
There are 4 distinct callers:
1. migration - explicitly set 'persistent' before calling. 2. libxl_driver First it assures that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. Then all usages of definition is under VIR_DOMAIN_VCPU_CONFIG flag, so 'persistent' is set.
3. virDomainObjCopyPersistentDef - all callers of this function has next structure: 1 call virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact thus we are sure that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. 2. call function of interest under VIR_DOMAIN_VCPU_CONFIG, so 'persistent' flag is set.
4. virDomainLiveConfigHelperMethod - the same reason as in 3.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
You've based your assumptions off of the change you made in 1/3 to virDomainObjUpdateModificationImpact regarding combining that if statement. Also even if the following would fail, you would have need to provide some sort of virReportError.
But that change is merely a refactoring it does not change behaviour. I'll add an error message of course but first I'll wait for agreement on that this patch is correct. May be I miss something as I really don't see why change you mention is matter.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e54c097..018c77e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2825,18 +2825,21 @@ virDomainObjSetDefTransient(virCapsPtr caps,
/* * Return the persistent domain configuration. If domain is transient, - * return the running config. + * return NULL. * * @param caps pointer to capabilities info * @param xmlopt pointer to XML parser configuration object * @param domain domain object pointer - * @return NULL on error, virDOmainDefPtr on success + * @return NULL on error or transient domains, virDOmainDefPtr on success */ virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain) { + if (!domain->persistent) + return NULL; + if (virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) return NULL;
ping On 02.02.2016 16:18, Nikolay Shirokovskiy wrote:
On 02.02.2016 15:13, John Ferlan wrote:
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
It's rather unexpected that function with such a name could return transient definition. This is possible if we call it for a transient active domain. But if it is called in such a conditions? No. So we would better fix this case to return NULL.
Calling conditions investigation.
There are 4 distinct callers:
1. migration - explicitly set 'persistent' before calling. 2. libxl_driver First it assures that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. Then all usages of definition is under VIR_DOMAIN_VCPU_CONFIG flag, so 'persistent' is set.
3. virDomainObjCopyPersistentDef - all callers of this function has next structure: 1 call virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact thus we are sure that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. 2. call function of interest under VIR_DOMAIN_VCPU_CONFIG, so 'persistent' flag is set.
4. virDomainLiveConfigHelperMethod - the same reason as in 3.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
You've based your assumptions off of the change you made in 1/3 to virDomainObjUpdateModificationImpact regarding combining that if statement. Also even if the following would fail, you would have need to provide some sort of virReportError.
But that change is merely a refactoring it does not change behaviour. I'll add an error message of course but first I'll wait for agreement on that this patch is correct. May be I miss something as I really don't see why change you mention is matter.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e54c097..018c77e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2825,18 +2825,21 @@ virDomainObjSetDefTransient(virCapsPtr caps,
/* * Return the persistent domain configuration. If domain is transient, - * return the running config. + * return NULL. * * @param caps pointer to capabilities info * @param xmlopt pointer to XML parser configuration object * @param domain domain object pointer - * @return NULL on error, virDOmainDefPtr on success + * @return NULL on error or transient domains, virDOmainDefPtr on success */ virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain) { + if (!domain->persistent) + return NULL; + if (virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) return NULL;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/02/2016 08:18 AM, Nikolay Shirokovskiy wrote:
On 02.02.2016 15:13, John Ferlan wrote:
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
It's rather unexpected that function with such a name could return transient definition. This is possible if we call it for a transient active domain. But if it is called in such a conditions? No. So we would better fix this case to return NULL.
Calling conditions investigation.
There are 4 distinct callers:
1. migration - explicitly set 'persistent' before calling. 2. libxl_driver First it assures that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. Then all usages of definition is under VIR_DOMAIN_VCPU_CONFIG flag, so 'persistent' is set.
3. virDomainObjCopyPersistentDef - all callers of this function has next structure: 1 call virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact thus we are sure that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. 2. call function of interest under VIR_DOMAIN_VCPU_CONFIG, so 'persistent' flag is set.
4. virDomainLiveConfigHelperMethod - the same reason as in 3.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
You've based your assumptions off of the change you made in 1/3 to virDomainObjUpdateModificationImpact regarding combining that if statement. Also even if the following would fail, you would have need to provide some sort of virReportError.
But that change is merely a refactoring it does not change behaviour. I'll add an error message of course but first I'll wait for agreement on that this patch is correct. May be I miss something as I really don't see why change you mention is matter.
Again sorry for the delay... I have a suspicion that there's a "hidden" meaning/side-effect to calling this... The whole usage of "newDef" vs "def" needs better nomenclature to stick in my long term memory. To refresh my short term though... The 'newDef' is the "Next" definition to activate at shutdown (of a running domain), while 'def' is currently active definition (whether it's running or not). Perhaps the function is less than adequately named considering that Persistent usually refers to on disk, although in this case, I'm wondering if it means whatever is active for the domain without having to have the caller know whether it's a "running" and whether it's taking it from disk. In the case of a transient domain, the persistent domain is only valid when "running" and there's no need for 'newDef', hence why virDomainObjSetDefTransient will make that check (and why virDomainObjGetPersistentDef will call it). I didn't follow in depth the instances you pointed out, but perhaps they are all that's left using the Get function. Maybe in the time since the Get function was created and now there's been code that's remove calls to the Get function and thus we've reached a point where we could do what you propose, but I haven't spent cycles researching all that. My point is - it's not only the current callers - what's the recent history on this? Has anything that used to call it now no longer call it and what was the reason? A caller in a more general code path that would want the 'def' returned for a transient domain - what would it call? I think that answer is virDomainObjGetDefs. If so, then the comment could be modified to indicate code paths that need to get the transient definition should call virDomainObjGetDefs (yet another aptly named method). John
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e54c097..018c77e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2825,18 +2825,21 @@ virDomainObjSetDefTransient(virCapsPtr caps,
/* * Return the persistent domain configuration. If domain is transient, - * return the running config. + * return NULL. * * @param caps pointer to capabilities info * @param xmlopt pointer to XML parser configuration object * @param domain domain object pointer - * @return NULL on error, virDOmainDefPtr on success + * @return NULL on error or transient domains, virDOmainDefPtr on success */ virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain) { + if (!domain->persistent) + return NULL; + if (virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) return NULL;
On 19.02.2016 02:19, John Ferlan wrote:
On 02/02/2016 08:18 AM, Nikolay Shirokovskiy wrote:
On 02.02.2016 15:13, John Ferlan wrote:
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
It's rather unexpected that function with such a name could return transient definition. This is possible if we call it for a transient active domain. But if it is called in such a conditions? No. So we would better fix this case to return NULL.
Calling conditions investigation.
There are 4 distinct callers:
1. migration - explicitly set 'persistent' before calling. 2. libxl_driver First it assures that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. Then all usages of definition is under VIR_DOMAIN_VCPU_CONFIG flag, so 'persistent' is set.
3. virDomainObjCopyPersistentDef - all callers of this function has next structure: 1 call virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact thus we are sure that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false. 2. call function of interest under VIR_DOMAIN_VCPU_CONFIG, so 'persistent' flag is set.
4. virDomainLiveConfigHelperMethod - the same reason as in 3.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
You've based your assumptions off of the change you made in 1/3 to virDomainObjUpdateModificationImpact regarding combining that if statement. Also even if the following would fail, you would have need to provide some sort of virReportError.
But that change is merely a refactoring it does not change behaviour. I'll add an error message of course but first I'll wait for agreement on that this patch is correct. May be I miss something as I really don't see why change you mention is matter.
Again sorry for the delay...
I have a suspicion that there's a "hidden" meaning/side-effect to calling this... The whole usage of "newDef" vs "def" needs better nomenclature to stick in my long term memory. To refresh my short term though... The 'newDef' is the "Next" definition to activate at shutdown (of a running domain), while 'def' is currently active definition (whether it's running or not).
Yes, it quite mangled place. I think we'd better stick with next definitions as this is how we seem to use them in other places. 1. transient config - config for a running domain, lost after it is destroyed. 2. persistent config - config on disk for a domain. Looking at the code I've found next correnspondence of def and newDef to this definitions: 1. active persistent domain def - transient newDef - persistent 2. inactive persistent domain def - persistent 3. transient domain (active) def - transient Thus newDef has only one purpose - persistent config of active persistent domain. def could be transient or persistent depends on situation. Now one can understand the reason for the function virDomainObjSetDefTransient naming. On persistent domain start we move from 'persistent' to 'transient' for 'def' meaining. 'live' flags helps us do it before actual start is happend.
Perhaps the function is less than adequately named considering that Persistent usually refers to on disk, although in this case, I'm wondering if it means whatever is active for the domain without having to have the caller know whether it's a "running" and whether it's taking it from disk.
I think such a function would be hard to use as usually we need to distinguish transient/persistent and have no notion of 'active'. Hmm ... if someone knows it is transient domain or inactive persistent domain it can refer to config without specifying it further as transient or persistent. Well in this case he can just use plain 'def' instead of this function. Another common usage of def is the case we need 'transient' config. It is always in 'def'. This is why we don't have virDomainObjGetTransientDef.
In the case of a transient domain, the persistent domain is only valid when "running" and there's no need for 'newDef', hence why virDomainObjSetDefTransient will make that check (and why virDomainObjGetPersistentDef will call it).
Yes that check is there because there is no need for newDef for a transient domain. But I think the reason virDomainObjGetPersistentDef calls virDomainObjSetDefTransient is different. 'Set' function is kind of lazy initialization tool. We initialize newDef only when someone wants change persistent config for a running persistent domain for the first time. This is the reason this check is there: if(domain->newDef) return 0; However I doubt we need this lazy initialization there anymore. After cb4c2694f166beca8f3e1fb37dedeec1ff3fbdf6 commit newDef is initialized early in the process of starting of domain for lxc and qemu drivers at least.
I didn't follow in depth the instances you pointed out, but perhaps they are all that's left using the Get function. Maybe in the time since the Get function was created and now there's been code that's remove calls to the Get function and thus we've reached a point where we could do what you propose, but I haven't spent cycles researching all that. My point is - it's not only the current callers - what's the recent history on this? Has anything that used to call it now no longer call it and what was the reason? A caller in a more general code path that would
It will take time to investigate it...
want the 'def' returned for a transient domain - what would it call? I think that answer is virDomainObjGetDefs. If so, then the comment could
Just get here. virDomainObjGetDefs has the exact description of correspondence I wrote above in one place. Here it is 'live' config instead of 'transient'.
be modified to indicate code paths that need to get the transient definition should call virDomainObjGetDefs (yet another aptly named method).
Yes, if someone needs 'def' for transient domain - it needs 'transient' config and can use this function. Hmm, one asks why bother to mention 'transient' on config for transient domain as there is no other one. Well looks like we have to as we have one type for all three states - active persistent, active transient, inactive persistent. As to reporting error upon return NULL from the function. I think it is not right as if someone use the return value in this circumstances - it is a case of incorrect code and moreover it will crash instanly. After all this investigation I would like to see one day all usages of def and newDef would be replaced by getTransient and getPersistent calls )) with all the mechanics incapsulated. Nikolay.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e54c097..018c77e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2825,18 +2825,21 @@ virDomainObjSetDefTransient(virCapsPtr caps,
/* * Return the persistent domain configuration. If domain is transient, - * return the running config. + * return NULL. * * @param caps pointer to capabilities info * @param xmlopt pointer to XML parser configuration object * @param domain domain object pointer - * @return NULL on error, virDOmainDefPtr on success + * @return NULL on error or transient domains, virDOmainDefPtr on success */ virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, virDomainObjPtr domain) { + if (!domain->persistent) + return NULL; + if (virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0) return NULL;
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy