[pve-devel] [RFC PATCH container] fix setting same netX value several times

[Dominik Csapak]

On 2/5/20 12:20 PM, Fabian Grünbichler wrote:
> On February 5, 2020 9:29 am, Dominik Csapak wrote:
>> when setting a netX option that is semantically the same as the
>> one already set but in a different order, e.g.:
>>
>> in config:
>> net0: name=eth0,bridge=vmbr0,hwaddr=AA:AA:AA:AA:AA:AA,type=veth
>> setting via api:
>> net0: bridge=vmbr0,name=eth0,hwaddr=AA:AA:AA:AA:AA:AA,type=veth
>>
>> the code tries to 'hot-apply' the change (which is no change really)
>> where the api line then gets parsed and printed which results
>> in the same string already in the config
>>
>> then we do a 'cleanup_pending' which removes it from pending, since
>> the config already contains the exact same options, but
>> then we overwrite the config from pending (which is empty)
>> resulting in an invalid config line:
>> --8<--
>> net0:
>> -->8--
>>
>> to prevent this, we only overwrite the config here when
>> there is still an option in in $conf->{pending}->{$opt}, meaning
>> there was a meaningful change
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>> i am not really sure if this is the correct place to fix this, because
>> i think we never should trigger apply_pending when the requested
>> config is semantically identical to what is already in the config,
>> but i did not really see when or how to do that in a good and generic way
>> (should be parse/print all property strings at the beginning?)
>> so sending it as an rfc
> 
> I think the right way to do it is like in QemuServer.pm - don't cleanup
> pending while iterating over it, but before/after iteration.
> 

sounds right, but i am not really versed in that code,
@Oguz can you maybe take a look again at this?
(since i think you worked on this last?)

if not i can do it, but i guess it would take me longer

>>   src/PVE/LXC/Config.pm | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
>> index 310aba6..49b9f70 100644
>> --- a/src/PVE/LXC/Config.pm
>> +++ b/src/PVE/LXC/Config.pm
>> @@ -1305,7 +1305,10 @@ sub vmconfig_apply_pending {
>>   	    $add_apply_error->($opt, $err);
>>   	} else {
>>   	    $class->cleanup_pending($conf);
>> -	    $conf->{$opt} = delete $conf->{pending}->{$opt};
>> +	    # $conf->{pending}->{$opt} is now empty if we have the same
>> +	    # value already in config, so do not overwrite the value in config
>> +	    $conf->{$opt} = delete $conf->{pending}->{$opt}
>> +		if defined($conf->{pending}->{$opt});
>>   	}
>>       }
>>   
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>