4:43 p.m.
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(a)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