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

["Michael S. Tsirkin" <mst@redhat.com>]

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:
> > > > > > > > > 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.
> > You don't need to have enum have same names to be compatible.
> > And in this case, all we really need is just a single number *2*
> > and a comment saying that's ESX server.
> I don't have to. I want to. It makes code much more clearer to reader. I
> don't see any harm in that.

It's just a bad interface for QEMU to use. Maybe it's good for vmware,
I would not know.

> > 
> > > This is no different than copying constants from a Linux device driver to
> > > implement it's device emulation in QEMU.
> > We really try to avoid stuff like this. If one does this one has to
> > check license etc etc.
> There is no license issue here. It's only definitions. And if you really
> wonder about it, this is the license written in the header files of
> open-vm-tools:
> /*********************************************************
>  * Copyright (C) 2006 VMware, Inc. All rights reserved.
>  *
>  * This program is free software; you can redistribute it and/or modify it
>  * under the terms of the GNU Lesser General Public License as published
>  * by the Free Software Foundation version 2.1 and no later version.

OK that is already a conflict with the license of vmport.c
which is copyleft. Respecting wishes of the original
author is not a legal requirement, but sure is a nice thing to do.

I suggest we keep clear of this.

Refer to it if you like but don't copy.

And "no later version" will conflict with a bunch of other
files which are 2 or later.
We can't avoid GPL v2 but we really shouldn't just add it
without any good reason.

>  * This program is distributed in the hope that it will be useful, but
>  * WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY
>  * or FITNESS FOR A PARTICULAR PURPOSE.  See the Lesser GNU General Public
>  * License for more details.
>  *
>  * You should have received a copy of the GNU Lesser General Public License
>  * along with this program; if not, write to the Free Software Foundation,
> Inc.,
>  * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA.
>  *
>  *********************************************************/
> > 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 


> And that is right and I will change it in v2. However,
> the rest doesn't violate coding style.
> In addition, I disagree this is confusing. These are official VMX product
> names defined by VMware.

They might make sense in the context of the specific project.
They aren't official names - just internal strings within a file.


> I don't see any value in renaming them. It just
> results in additional confusion.
> > 
> > 
> > > > 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.
> > What good does this do when that code will change tomorrow?
> Why would the enum constants change tomorrow?
> And even if that will happen, it still allows a reader to just search in
> Google the name of the constant and find results.
> Which is better than just making up names that we think our more intuitive
> than the names VMware decided for their own product.
> > 
> > You worry about code being easy to write, I worry about it
> > being easy to read.
> 
> No I don't. This doesn't matter at all for writing code but matters only to
> reading it.
> 
> > 
> > Here are things we can do to make things easier for users and readers:
> > - use full name VM executable instead of VMX
> Why? Searching for "VMware VM executable" in Google provides completely
> unrelated results.
> In contrast, searching for "VMware VMX" provides concrete related results.
> We shouldn't rename terminology given by VMware itself to it's own product.
> It just adds confusion in my opinion.
> > - put in official product names in comments instead of enums
> I don't see how it provides extra value. Especially due to the fact that the
> enum constants have their more common product name next to them in comment.
> I provide both reference that can be searched in other VMware projects and
> web and the more user-friendly well-known name.
> > - write code using our coding style
> Will do. The only coding style violation I see here is the enum type name.
> Will change from "VMX_Type" to "VMXType".
> The rest seems not violating coding convention. Please tell me if I missed
> something.
> > - add a link to where you found a specific number in comments
> Good idea. Will add a link to open-vm-tools git repo in vmport.c comment in
> general.
> > 
> > 
> > 
> > 
> > 
> > 
> > > > > > > > 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...
> > 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 ...




> > 
> > > > 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.
> > Official as in will stick around, not official as in pushed to
> > a github repo.
> > 
> > 
> > > 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?
Oh because we lifted this wholesale from some other header.

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

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


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

-- 
MST