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

[Liran Alon <liran.alon@oracle.com>]

On 10/03/2020 19:36, Michael S. Tsirkin wrote:
> 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.
It lists all currently known values for this enum. Exactly as you do in 
your comment...
The only way to prevent user from entering a value that is not defined 
in enum is to restrict user to enum-values.
Which can be done if you think it's appropriate. I think it just limits 
production flexibility.
>
>> 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.
The "guest driver" in this case is open-vm-tools. And it helps that the 
names match.
> 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 ...
The "driver" in this case is open-vm-tools.
The legacy guests have proprietary drivers that mimics VMware Tools or a 
subset of it's functionality.
>
>
>   * See https://urldefense.com/v3/__https://github.com/vmware/open-vm-tools/blob/master/open-vm-tools/lib/include/vm_vmx_type.h__;!!GqivPVa7Brio!IuRMdod4d33nvVKOiG-itVXtxnHA9nAouQdYDxv3E62rIzVzPBWZ5M54D7BEF3g$
>   * 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.
I still don't understand why you view a comment to be better than an enum.
This also contradict the approach taken for other enums in device 
emulation code, as I have provided multiple examples in previous reply.
>
>
>
>> 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.

Which part of license to I ignore?

I fixed all the comments you have mentioned beside this thing that is 
debatable. I wish to hear an additional opinion.
If you really strongly insist on this, I can change this to what you 
want without further discussion...

Why not allow to review all the 15 patches besides a discussion of 
whether constants should be defined in enum or comment?
That seems a little harsh to me.

-Liran