[pve-devel] applied: [PATCH manager] ui: vm/HDEdit: fix backup checkbox default

[Thomas Lamprecht]

On 12/10/19 10:28 AM, Aaron Lauterer wrote:
> Have you also adapted the VZDump code accordingly?

That cannot be done without breaking backward compat.

> 
> AFAIU for Qemu VMs the default behavior, in absence of a backup flag for the disk, is to include it in the backup. Which is why if the "No Backup" checkbox was set, the "backup=0" flag was added.
> 
> I tried to change the checkbox and it's behavior to adhere to this.

I mean, yeah, the "set on default" was a bit missleading for me here,
as we normally don't do that, so the fix to get it all as before
but with reveresed checkbox ("Backup" vs. "Nobackup") was to additionally
do:

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index 40055442..fd890600 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -77,7 +77,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
            me.drive.format = values.diskformat;
        }
 
-       PVE.Utils.propertyStringSet(me.drive, values.backup, 'backup');
+       PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
        PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
        PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
        PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
@@ -130,7 +130,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
        }
 
        values.hdimage = drive.file;
-       values.backup = PVE.Parser.parseBoolean(drive.backup, 0);
+       values.backup = PVE.Parser.parseBoolean(drive.backup, 1);
        values.noreplicate = !PVE.Parser.parseBoolean(drive.replicate, 1);
        values.diskformat = drive.format || 'raw';
        values.cache = drive.cache || '__default__';

> 
> This patch it breaking it unless I miss something.
> 

You are right, much thanks for hollering!

cheers,
Thomas

> All the best,
> Aaron
> 
> On 12/9/19 3:45 PM, Thomas Lamprecht wrote:
>> The recent change transformed this checkbox from a negative "No
>> Backup" to a positive "Backup" did not accounted for changing the
>> default fallback for the parseBoolean call.
>>
>> As this would had suggested that disk are backed-up which aren't
>> backup, it could lead to pretty devastating results...
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>   www/manager6/qemu/HDEdit.js | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
>> index 8d469f6f..40055442 100644
>> --- a/www/manager6/qemu/HDEdit.js
>> +++ b/www/manager6/qemu/HDEdit.js
>> @@ -130,7 +130,7 @@ Ext.define('PVE.qemu.HDInputPanel', {
>>       }
>>         values.hdimage = drive.file;
>> -    values.backup = PVE.Parser.parseBoolean(drive.backup, 1);
>> +    values.backup = PVE.Parser.parseBoolean(drive.backup, 0);
>>       values.noreplicate = !PVE.Parser.parseBoolean(drive.replicate, 1);
>>       values.diskformat = drive.format || 'raw';
>>       values.cache = drive.cache || '__default__';
>> @@ -296,7 +296,6 @@ Ext.define('PVE.qemu.HDInputPanel', {
>>               tag: 'div',
>>               'data-qtip': gettext('Include volume in backup job'),
>>           },
>> -        uncheckedValue: 'no',
>>           labelWidth: labelWidth,
>>           name: 'backup',
>>           bind: {
>>