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@xxxxxxxxxxxxx>
>>>> ---
>>>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list