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

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

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:
>>>>>> As can be seen from VmCheck_GetVersion() in open-vm-tools code,
>>>>>> CMD_GETVERSION should return VMX type in ECX register.
>>>>>>
>>>>>> Default is to fake host as VMware ESX server. But user can control
>>>>>> this value by "-global vmport.vmx-type=X".
>>>>>>
>>>>>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>>>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>>>>> ---
>>>>>>     hw/i386/vmport.c | 13 +++++++++++++
>>>>>>     1 file changed, 13 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
>>>>>> index a2c8ff4b59cf..c03f57f2f636 100644
>>>>>> --- a/hw/i386/vmport.c
>>>>>> +++ b/hw/i386/vmport.c
>>>>>> @@ -36,6 +36,15 @@
>>>>>>     #define VMPORT_ENTRIES 0x2c
>>>>>>     #define VMPORT_MAGIC   0x564D5868
>>>>>> +typedef enum {
>>>>>> +   VMX_TYPE_UNSET = 0,
>>>>>> +   VMX_TYPE_EXPRESS,    /* Deprecated type used for VMware Express */
>>>>>> +   VMX_TYPE_SCALABLE_SERVER,    /* VMware ESX server */
>>>>>> +   VMX_TYPE_WGS,        /* Deprecated type used for VMware Server */
>>>>>> +   VMX_TYPE_WORKSTATION,
>>>>>> +   VMX_TYPE_WORKSTATION_ENTERPRISE /* Deprecated type used for ACE 1.x */
>>>>>> +} VMX_Type;
>>>>>> +
>>>>> Is this really VMX type? And do users care what it is?
>>>> This enum is copied from open-vm-tools source code
>>>> (lib/include/vm_version.h). This is how it's called in VMware Tools
>>>> terminology... Don't blame me :)
>>> I don't even want to go look at it to check license compatibility, but
>>> IMHO that's just another reason to avoid copying it.
>>> Copying bad code isn't a good idea unless needed for
>>> compatibility.
>> Preserving original VMware terminology makes sense and is preferred in my
>> opinion. I think diverging from it is more confusing.
> Yea tell it to people who got in hot water because they copied
> some variable names to avoid confusion. Oh wait.
>
> This is not an official terminology I think.
Maybe it wasn't clear from my previous messages, but open-vm-tools is an 
official VMware open-source project...
VMX is the official name of the VMware Userspace-VMM and VMX-Type is an 
official name as-well.

I'm also not copying code here... I'm copying definitions from relevant 
header files to implement a compatible interface.
This is no different than copying constants from a Linux device driver 
to implement it's device emulation in QEMU.

> So please just make it make sense by itself, and make it
> easy to research.
I think I have made it the most easiest to research. Having exactly same 
names as VMware official project and pointing to it directly from 
comments and commit messages.
>
>>>
>>>>> Also, how about friendlier string values so people don't need to
>>>>> figure out code numbers?
>>>> I could have defined a new PropertyInfo struct in hw/core/qdev-properties.c
>>>> for this enum and then define a proper macro in qdev-properties.h.
>>>> But it seems like an overkill for a value that is suppose to rarely be
>>>> changed. So I thought this should suffice for now for user-experience
>>>> perspective.
>>>> If you think otherwise, I can do what I just suggested above.
>>>>
>>>> -Liran
>>> I think that's better, and this allows you to use official
>>> product names that people can relate to.
>> Ok. Will do...
>>> Alternatively just drop this enum completely.  As far as you are
>>> concerned it's just a number VM executable gives together with the
>>> version, right?  We don't even need the enum, just set it to 2 and add a
>>> code comment saying it's esx server.
>> I could do the latter alternative but why? It just hides information
>> original patch author (myself) know about where this value comes from.
>> I don't see a reason to hide information from future code maintainers.
>> Similar to defining all flags of a given flag-field even if we use only a
>> subset of it.
>>
>> -Liran
> That belongs in a code comment. Removes need to follow silly names from
> unrelated and possibly incompatible license.

What do you mean "unrelated"? It's an official VMware open-source 
project for VMware Tools...
I'm only copying definition of constants...

> By comparison dead code is
> dead code.
Right. That's why I think the enum PropertyInfo mechanism is an overkill 
at this point.
> But sure, if you want to code up user friendly names, that's
> ok too. But do follow official names then please, not something lifted
> from some piece of code.

These are all official names. I'm not sure I understand what you are 
suggesting.

-Liran