Re: [PATCH pve-manager v2 4/4] partially fix #7801: ui: hardware view: warn about problematic virtio versions

["Nicolas Frey" <n.frey@proxmox.com>]

On Wed Aug 26, 2026 at 1:44 PM CEST, Fiona Ebner wrote:
> Am 26.08.26 um 9:49 AM schrieb Nicolas Frey:
>> based on whether the ostype is windows or not. extends the
>> `editorFactory` to allow extraOptions to be a callback to allow for lazy
>> evaluation. this is needed as 'ostype' is not yet instantiated when
>> eagerly evaluating.
>>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7801
>> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
>> ---
>> changes since v1:
>> * actually lazily evaluate the ostype by putting it in
>>   the function body that gets returned (i.e. the factory)
>> * add missing warnVirtio to commonOpts
>>
>>  www/manager6/qemu/HardwareView.js | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
>> index e6c02299..9e900746 100644
>> --- a/www/manager6/qemu/HardwareView.js
>> +++ b/www/manager6/qemu/HardwareView.js
>> @@ -436,6 +436,7 @@ Ext.define('PVE.qemu.HardwareView', {
>>                  listeners: {
>>                      destroy: () => me.reload(),
>>                  },
>> +                warnVirtio: PVE.Utils.is_windows(me.getObjectValue('ostype')),
>
> Seems a bit out of place in the commonOpts. Can't we do this more
> locally in the definition below? Might need splitting out like the
> efidisk_menuitem?
>

doesn't efidisk_menuitem just apply to the "Add" dropdown menu? the commonOpts
are IMO not ideal, but there is no option to pass in any additional options:

```
if (Ext.isString(editor)) {
    Ext.create(editor, commonOpts);
}
```

where the `CDEdit` editor is determined (in `run_editor`) like so:

```
if (rowdef.isOnStorageBus) {
    let value = me.getObjectValue(rec.data.key, '', true);
    if (isCloudInitKey(value)) {
        return;
    } else if (value.match(/media=cdrom/)) {
        editor = 'PVE.qemu.CDEdit';
    } else if (!diskCap) {
        return;
    }
}
```

I'd have to rewrite a bunch of stuff here to allow for that and I'm not
sure it's worth the hassle here. what do you think?

>>              };
>>
>>              if (Ext.isString(editor)) {
>> @@ -805,7 +806,6 @@ Ext.define('PVE.qemu.HardwareView', {
>>          };
>>
>>          let editorFactory = (classPath, extraOptions) => {
>> -            extraOptions = extraOptions || {};
>>              return () =>
>>                  Ext.create(`PVE.qemu.${classPath}`, {
>>                      autoShow: true,
>> @@ -816,7 +816,7 @@ Ext.define('PVE.qemu.HardwareView', {
>>                      },
>>                      isAdd: true,
>>                      isCreate: true,
>> -                    ...extraOptions,
>> +                    ...(Ext.isFunction(extraOptions) ? extraOptions() : extraOptions || {}),
>
> This is rather hacky too.
>

do you mean the way it's written with a ternary or generally the lazy evaluation?

>>                  });
>>          };
>>
>> @@ -847,7 +847,9 @@ Ext.define('PVE.qemu.HardwareView', {
>>                                  text: gettext('CD/DVD Drive'),
>>                                  iconCls: 'pve-itype-icon-cdrom',
>>                                  disabled: !caps.vms['VM.Config.CDROM'],
>> -                                handler: editorFactory('CDEdit'),
>> +                                handler: editorFactory('CDEdit', () => ({
>> +                                    warnVirtio: PVE.Utils.is_windows(me.getObjectValue('ostype')),
>> +                                })),
>>                              },
>>                              {
>>                                  text: gettext('Network Device'),
>> --
>> 2.47.3
>>
>>
>>