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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 26.08.26 um 3:37 PM schrieb Nicolas Frey:
> 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?

Hmm, right.

> 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';

Can we define an instance with the extra option up-front and then assign
that one instead of the string here?

>     } 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?
> 

I meant the overloading of the parameter to either be a dictionary or a
function. But to be fair, it is JavaScript, so nothing unheard of.

>>>                  });
>>>          };
>>>
>>> @@ -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
>>>
>>>
>>>