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

[Liran Alon]

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 <address@hidden>
> > > > > > Signed-off-by: Liran Alon <address@hidden>
> > > > > > ---
> > > > > >     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