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

[Michael S. Tsirkin]

On Tue, Mar 10, 2020 at 06:39:33PM +0200, Liran Alon wrote:
> > > 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.

That's not factually correct in this case. In C, enum does not
necessarily list all possible values generally. Neither is it always
used like this in QEMU. Neither does it do it in this case, nothing
prevents user from sticking any single-byte value in the property.

> 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.

Yes, it's sometimes helpful to match guest driver since that helps debug
the whole stack. There's literally nothing to help debug here though.
But if you feel strongly, here's a conversation starter.
But it raises some questions that need to be answered
properly:


/*
 * Virtual Machine eXecutable type (VMX).
 *
 * Most guests are fine with the default.
 *
 * Some legacy guests hard-code a given type.

^ Is this the real reason we are including this option?
Because if it is how is it helpful to add link to
the open-source drivers? These likely are not legacy ...


 * See 
https://github.com/vmware/open-vm-tools/blob/master/open-vm-tools/lib/include/vm_vmx_type.h
 * for an up-to-date list of values.
 * To help locate relevant portions of guest driver code
 * and debug guest failures, enum names from the above header
 * are listed below:
 *
 * Reasonable options:
 * 0 - unset? - see VMX_TYPE_UNSET

                        ^^^ Note as you know what this is, please write it up.

 * 1 - VMware Express (deprecated) - see VMX_TYPE_UNSET
 * 2 - VMware ESX server - see VMX_TYPE_EXPRESS
 * 3 - VMware Server (deprecated) - see VMX_TYPE_WGS
 * 4 - VMware Workstation - see VMX_TYPE_WORKSTATION
 * 5 - ACE 1.x (deprecated) - see VMX_TYPE_WORKSTATION_ENTERPRISE
 */
DEFINE_PROP_UINT8("vm-executable-type", VMPortState, vm_executable_type, 2),


Maybe above is OK, if above questions can be addressed.



> 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.

Sorry, if you don't address my comments from v1 please do not expect me
to review v2.  I also feel strongly about proper attribution.  Ignoring
original license on vmport.c making it depend on "GPL v2 but not later"
bits for cosmetic reasons just isn't right.


-- 
MST