qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Liran Alon
Subject: Re: [PATCH 05/14] hw/i386/vmport: Report VMX type in CMD_GETVERSION
Date: Tue, 10 Mar 2020 16:46:19 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 

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

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. 


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.
 *
 * 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". 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. 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?



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? Isn't enum invented exactly for enumerating all possible values of a field? 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.
And again, I disagree with renaming the field to "vm-executable-type" instead of "vmx-type". 

-Liran
reply via email to
[Prev in Thread] Current Thread [Next in Thread]