12:42 a.m.
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(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...
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