Re: [PATCH 05/14] hw/i386/vmport: Report VMX type in CMD_GETVERSION

[Liran Alon]

On 10/03/2020 17:10, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 04:46:19PM +0200, Liran Alon wrote:
> > On 10/03/2020 16:08, Michael S. Tsirkin wrote:
> > > On Tue, Mar 10, 2020 at 03:35:25PM +0200, Liran Alon wrote:
> > > > On 10/03/2020 14:53, Michael S. Tsirkin wrote:
> > > > > On Tue, Mar 10, 2020 at 02:43:51PM +0200, Liran Alon wrote:
> > > > > > On 10/03/2020 14:35, Michael S. Tsirkin wrote:
> > > > > > > On Tue, Mar 10, 2020 at 02:25:28PM +0200, Liran Alon wrote:
> > > > > > > > On 10/03/2020 14:14, Michael S. Tsirkin wrote:
> > > > > > > > > On Tue, Mar 10, 2020 at 01:54:02AM +0200, Liran Alon wrote:
> > > But in this case, the names are confusing,
> > > violate our coding style, I could go on.
> > The only thing that violates the coding style is "VMX_Type" enum type name
> > instead of "VMXType".
> All enum names too. Supposed to be CamelCase. Again VMX is

Looking at other enums defined in QEMU, it doesn't seem that constant 
names are CamelCase. Only the enum type name.
E.g. PVSCSIRegOffset, BiosAtaTranslation and etc.

> > > No you also copy names and comments. Which might make sense in the
> > > context of the original project but seem to make no sense here.
> > > E.g. for vmware a given product is deprecated but why does QEMU care?
> > What is the harm in specifying that? It gives more context.
> > > enum values are not even listed. What is poor user supposed to do -
> > > take out a calculator to figure it out?
> > What do you mean by listed?
> So imagine: as a user, I want to set this to some reasonable value.
>
> Supposedly this is why you have the enum there in the
> 1st place right? Let's see how does all this help me:
>
> - first enum is VMX_TYPE_UNSET. Unset? I guess that's
> the default. I want to set it, make sure it's a good value.
> - next one is VMX_TYPE_EXPRESS. comment says deprecated though.
>    I will keep clear.
> - Next enum is VMX_TYPE_SCALABLE_SERVER. Hmm that says ESX.
> I guess it's good! However what's scalable server?
> There's no vmware in sight,
> brings up unrelated search results.
> Scalable server? No I need to research that.
> I guess I will just ignore all this and go by the comments.
> Okay! Wait so what is the value I need to supply to the
> property?
> Oh right I need to recall that enum values are sequential.
> So first one it says is 0. Let me count. It's 2 I guess.
>
> Okay I will try ...

The person who is expected to manipulate this property is quite advance 
to begin with...
The process described above is quite simple for such person.

> > > > I'm not sure I understand what you are
> > > > suggesting.
> > > >
> > > > -Liran
> > > Something like the below.
> > >
> > > /*
> > >    * Most guests are fine with the default.
> > >    * Some legacy guests hard-code a given type.
> > >    * See 
> > > https://urldefense.com/v3/__https://github.com/vmware/open-vm-tools/blob/master/open-vm-tools/lib/include/vm_vmx_type.h__;!!GqivPVa7Brio!M9wko4CSBSs3xFA2QY7MIL_jvAxlU5aRZE1jN2hzG5jnk8rdlpYCDs2ymrkJ8GE$
> > >    * for an up-to-date list of values.
> > >    *
> > >    * Reasonable options:
> > >    * 0 - unset?
> > >    * 1 - VMware Express (deprecated)
> > >    * 2 - VMware ESX server
> > >    * 3 - VMware Workstation
> > >    * 4 - ACE 1.x (deprecated)
> > >    */
> > >
> > > DEFINE_PROP_UINT8("vm-executable-type", VMPortState, vm_executable_type, 2 /* 
> > > VMware ESX server */),
> > Why is it better to specify a list of all options in a comment than an enum?
> Because that lets you use english. Look you didn't even list options.
> User's supposed to do the math in his/her head. Why is that?
Figuring out an enum value from definition is trivial. If you wish, I 
can change those to #define to make it more clear but error-prone.
> Oh because we lifted this wholesale from some other header.
That's not the reason.
> > Isn't enum invented exactly for enumerating all possible values of a field?
> No - it just assigns names to constants. If you then proceed not to use
> the names, then it's pointless.
It's not. It exactly lists all the various possible values. In contrast 
to directing the reader to a link (which may be broken in the future),
to figure out from there what should be the values. That seems more 
annoying to me as a reader.
> > Note that even in this simple case, you needed to write "VMware ESX server"
> > twice instead of referring to an enum constant. It doesn't seem more elegant
> > to me.
> I felt this bears repetition.
> But sure, you can drop it in DEFINE_PROP_UINT8 if you like.
> If you really feel you must, do:
>
> #define VM_PORT_DEFAULT_VM_EXECUTABLE 2
> near the comment.

Why not just define the entire enum then?...

This approach seems quite common for all device emulation code.
E.g. BTSTAT_HATIMEOUT in HostBusAdapterStatus, 
PVSCSI_REG_OFFSET_LAST_STS_3 in PVSCSIRegOffset, VMXNET3_CMD_UPDATE_IML 
in vmxnet3.h and etc.

> > And again, I disagree with renaming the field to "vm-executable-type"
> > instead of "vmx-type".
> >
> > -Liran
> Acronims is a bad idea in user interfaces if avoidable, or unless
> universal. Either these interfaces are needed or they aren't.
> I question their usefulness, but if they are useful they should
> have names that do not require guesswork to understand.
Giving new names to existing terminology that can be matched against 
existing guest code which interface with your device emulation is what 
requires guesswork.
Using names matching the guest code driver is what doesn't require 
guesswork and is more intuitive to understand.

Let's agree that I will fix coding convention issue (VMX_Type -> 
VMXType) and link to open-vm-tools but remain with the enum.
And see what other maintainers have to see about this on v2.

-Liran