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;
+
#define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
typedef struct VMPortState {
@@ -46,6 +55,7 @@ typedef struct VMPortState {
void *opaque[VMPORT_ENTRIES];
uint32_t vmx_version;
+ uint8_t vmx_type;
} VMPortState;
static VMPortState *port_state;
@@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
X86CPU *cpu = X86_CPU(current_cpu);
cpu->env.regs[R_EBX] = VMPORT_MAGIC;
+ cpu->env.regs[R_ECX] = port_state->vmx_type;
return port_state->vmx_version;
}
@@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
static Property vmport_properties[] = {
/* Default value taken from open-vm-tools code VERSION_MAGIC definition */
DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
+ DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
+ VMX_TYPE_SCALABLE_SERVER),
DEFINE_PROP_END_OF_LIST(),
};
--
2.20.1
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;
> +
Can names be prefixed with VMPort pls? VMX has specific unrelated meaning.
Same everywhere.
> #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
>
> typedef struct VMPortState {
> @@ -46,6 +55,7 @@ typedef struct VMPortState {
> void *opaque[VMPORT_ENTRIES];
>
> uint32_t vmx_version;
> + uint8_t vmx_type;
> } VMPortState;
>
> static VMPortState *port_state;
> @@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
> X86CPU *cpu = X86_CPU(current_cpu);
>
> cpu->env.regs[R_EBX] = VMPORT_MAGIC;
> + cpu->env.regs[R_ECX] = port_state->vmx_type;
> return port_state->vmx_version;
> }
>
> @@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
> static Property vmport_properties[] = {
> /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
> DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
> + DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
> + VMX_TYPE_SCALABLE_SERVER),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> --
> 2.20.1
On 10/03/2020 11:20, 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;
>> +
> Can names be prefixed with VMPort pls? VMX has specific unrelated meaning.
>
> Same everywhere.
I didn't thought it matters much given that this enum is only defined
locally in vmport.c.
But sure I can rename it in v2.
-Liran
>
>> #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
>>
>> typedef struct VMPortState {
>> @@ -46,6 +55,7 @@ typedef struct VMPortState {
>> void *opaque[VMPORT_ENTRIES];
>>
>> uint32_t vmx_version;
>> + uint8_t vmx_type;
>> } VMPortState;
>>
>> static VMPortState *port_state;
>> @@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
>> X86CPU *cpu = X86_CPU(current_cpu);
>>
>> cpu->env.regs[R_EBX] = VMPORT_MAGIC;
>> + cpu->env.regs[R_ECX] = port_state->vmx_type;
>> return port_state->vmx_version;
>> }
>>
>> @@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
>> static Property vmport_properties[] = {
>> /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
>> DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
>> + DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
>> + VMX_TYPE_SCALABLE_SERVER),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> --
>> 2.20.1
On Tue, Mar 10, 2020 at 01:18:44PM +0200, Liran Alon wrote:
>
> On 10/03/2020 11:20, 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;
> > > +
> > Can names be prefixed with VMPort pls? VMX has specific unrelated meaning.
> >
> > Same everywhere.
> I didn't thought it matters much given that this enum is only defined
> locally in vmport.c.
> But sure I can rename it in v2.
>
> -Liran
Property names matter more.
> >
> > > #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
> > > typedef struct VMPortState {
> > > @@ -46,6 +55,7 @@ typedef struct VMPortState {
> > > void *opaque[VMPORT_ENTRIES];
> > > uint32_t vmx_version;
> > > + uint8_t vmx_type;
> > > } VMPortState;
> > > static VMPortState *port_state;
> > > @@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
> > > X86CPU *cpu = X86_CPU(current_cpu);
> > > cpu->env.regs[R_EBX] = VMPORT_MAGIC;
> > > + cpu->env.regs[R_ECX] = port_state->vmx_type;
> > > return port_state->vmx_version;
> > > }
> > > @@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
> > > static Property vmport_properties[] = {
> > > /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
> > > DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
> > > + DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
> > > + VMX_TYPE_SCALABLE_SERVER),
> > > DEFINE_PROP_END_OF_LIST(),
> > > };
> > > --
> > > 2.20.1
On 10/03/2020 13:23, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 01:18:44PM +0200, Liran Alon wrote:
>> On 10/03/2020 11:20, 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;
>>>> +
>>> Can names be prefixed with VMPort pls? VMX has specific unrelated meaning.
>>>
>>> Same everywhere.
>> I didn't thought it matters much given that this enum is only defined
>> locally in vmport.c.
>> But sure I can rename it in v2.
>>
>> -Liran
> Property names matter more.
You mean to rename "vmx-version" and "vmx-type" to "vmport-vmx-version"
and "vmport-vmx-type"?
They are properties of vmport object so it seems redundant no? Also
doesn't seem consistent which how properties of other objects in QEMU
are named. (E.g. PVSCSI have "use_msg" property. Not "pvscsi_use_msg").
But will do as you will suggest. Just asking for guidance of what you
are looking for.
-Liran
>
>
>>>> #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
>>>> typedef struct VMPortState {
>>>> @@ -46,6 +55,7 @@ typedef struct VMPortState {
>>>> void *opaque[VMPORT_ENTRIES];
>>>> uint32_t vmx_version;
>>>> + uint8_t vmx_type;
>>>> } VMPortState;
>>>> static VMPortState *port_state;
>>>> @@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
>>>> X86CPU *cpu = X86_CPU(current_cpu);
>>>> cpu->env.regs[R_EBX] = VMPORT_MAGIC;
>>>> + cpu->env.regs[R_ECX] = port_state->vmx_type;
>>>> return port_state->vmx_version;
>>>> }
>>>> @@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
>>>> static Property vmport_properties[] = {
>>>> /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
>>>> DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
>>>> + DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
>>>> + VMX_TYPE_SCALABLE_SERVER),
>>>> DEFINE_PROP_END_OF_LIST(),
>>>> };
>>>> --
>>>> 2.20.1
On Tue, Mar 10, 2020 at 01:40:24PM +0200, Liran Alon wrote:
>
> On 10/03/2020 13:23, Michael S. Tsirkin wrote:
> > On Tue, Mar 10, 2020 at 01:18:44PM +0200, Liran Alon wrote:
> > > On 10/03/2020 11:20, 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;
> > > > > +
> > > > Can names be prefixed with VMPort pls? VMX has specific unrelated meaning.
> > > >
> > > > Same everywhere.
> > > I didn't thought it matters much given that this enum is only defined
> > > locally in vmport.c.
> > > But sure I can rename it in v2.
> > >
> > > -Liran
> > Property names matter more.
> You mean to rename "vmx-version" and "vmx-type" to "vmport-vmx-version" and
> "vmport-vmx-type"?
> They are properties of vmport object so it seems redundant no? Also doesn't
> seem consistent which how properties of other objects in QEMU are named.
> (E.g. PVSCSI have "use_msg" property. Not "pvscsi_use_msg").
> But will do as you will suggest. Just asking for guidance of what you are
> looking for.
>
> -Liran
Sorry - no I'm looking for an alternative to "vmx" everywhere but especially
in property names which need to be maintained. Maybe vm-exec or vmexec
as I suggested separately.
> >
> >
> > > > > #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
> > > > > typedef struct VMPortState {
> > > > > @@ -46,6 +55,7 @@ typedef struct VMPortState {
> > > > > void *opaque[VMPORT_ENTRIES];
> > > > > uint32_t vmx_version;
> > > > > + uint8_t vmx_type;
> > > > > } VMPortState;
> > > > > static VMPortState *port_state;
> > > > > @@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
> > > > > X86CPU *cpu = X86_CPU(current_cpu);
> > > > > cpu->env.regs[R_EBX] = VMPORT_MAGIC;
> > > > > + cpu->env.regs[R_ECX] = port_state->vmx_type;
> > > > > return port_state->vmx_version;
> > > > > }
> > > > > @@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
> > > > > static Property vmport_properties[] = {
> > > > > /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
> > > > > DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
> > > > > + DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
> > > > > + VMX_TYPE_SCALABLE_SERVER),
> > > > > DEFINE_PROP_END_OF_LIST(),
> > > > > };
> > > > > --
> > > > > 2.20.1
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?
Also, how about friendlier string values so people don't need to
figure out code numbers?
> #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
>
> typedef struct VMPortState {
> @@ -46,6 +55,7 @@ typedef struct VMPortState {
> void *opaque[VMPORT_ENTRIES];
>
> uint32_t vmx_version;
> + uint8_t vmx_type;
> } VMPortState;
>
> static VMPortState *port_state;
> @@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
> X86CPU *cpu = X86_CPU(current_cpu);
>
> cpu->env.regs[R_EBX] = VMPORT_MAGIC;
> + cpu->env.regs[R_ECX] = port_state->vmx_type;
> return port_state->vmx_version;
> }
>
> @@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
> static Property vmport_properties[] = {
> /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
> DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
> + DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
> + VMX_TYPE_SCALABLE_SERVER),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> --
> 2.20.1
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 :)
> 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
>
>> #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
>>
>> typedef struct VMPortState {
>> @@ -46,6 +55,7 @@ typedef struct VMPortState {
>> void *opaque[VMPORT_ENTRIES];
>>
>> uint32_t vmx_version;
>> + uint8_t vmx_type;
>> } VMPortState;
>>
>> static VMPortState *port_state;
>> @@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
>> X86CPU *cpu = X86_CPU(current_cpu);
>>
>> cpu->env.regs[R_EBX] = VMPORT_MAGIC;
>> + cpu->env.regs[R_ECX] = port_state->vmx_type;
>> return port_state->vmx_version;
>> }
>>
>> @@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
>> static Property vmport_properties[] = {
>> /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
>> DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
>> + DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
>> + VMX_TYPE_SCALABLE_SERVER),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> --
>> 2.20.1
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.
> > 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.
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.
> >
> > > #define VMPORT(obj) OBJECT_CHECK(VMPortState, (obj), TYPE_VMPORT)
> > > typedef struct VMPortState {
> > > @@ -46,6 +55,7 @@ typedef struct VMPortState {
> > > void *opaque[VMPORT_ENTRIES];
> > > uint32_t vmx_version;
> > > + uint8_t vmx_type;
> > > } VMPortState;
> > > static VMPortState *port_state;
> > > @@ -114,6 +124,7 @@ static uint32_t vmport_cmd_get_version(void *opaque, uint32_t addr)
> > > X86CPU *cpu = X86_CPU(current_cpu);
> > > cpu->env.regs[R_EBX] = VMPORT_MAGIC;
> > > + cpu->env.regs[R_ECX] = port_state->vmx_type;
> > > return port_state->vmx_version;
> > > }
> > > @@ -173,6 +184,8 @@ static void vmport_realizefn(DeviceState *dev, Error **errp)
> > > static Property vmport_properties[] = {
> > > /* Default value taken from open-vm-tools code VERSION_MAGIC definition */
> > > DEFINE_PROP_UINT32("vmx-version", VMPortState, vmx_version, 6),
> > > + DEFINE_PROP_UINT8("vmx-type", VMPortState, vmx_type,
> > > + VMX_TYPE_SCALABLE_SERVER),
> > > DEFINE_PROP_END_OF_LIST(),
> > > };
> > > --
> > > 2.20.1
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.
>
>
>>> 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
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.
So please just make it make sense by itself, and make it
easy to research.
> >
> >
> > > > 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. By comparison dead code is
dead code. 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.
--
MST
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
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.
> 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. But in this case, the names are confusing,
violate our coding style, I could go on.
> > 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?
You worry about code being easy to write, I worry about it
being easy to read.
Here are things we can do to make things easier for users and readers:
- use full name VM executable instead of VMX
- put in official product names in comments instead of enums
- write code using our coding style
- add a link to where you found a specific number in comments
> >
> > > >
> > > > > > 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?
enum values are not even listed. What is poor user supposed to do -
take out a calculator to figure it out?
> > 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://github.com/vmware/open-vm-tools/blob/master/open-vm-tools/lib/include/vm_vmx_type.h
* 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 */),
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.
>
>> 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
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
On 10/03/2020 17:10, Michael S. Tsirkin wrote:
> 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:
>>> 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
Looking at other enums defined in QEMU, it doesn't seem that constant
names are CamelCase. Only the enum type name.
E.g. PVSCSIRegOffset, BiosAtaTranslation and etc.
>>> 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 ...
The person who is expected to manipulate this property is quite advance
to begin with...
The process described above is quite simple for such person.
>>>
>>>> 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?
Figuring out an enum value from definition is trivial. If you wish, I
can change those to #define to make it more clear but error-prone.
> Oh because we lifted this wholesale from some other header.
That's not the reason.
>> 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. In contrast
to directing the reader to a link (which may be broken in the future),
to figure out from there what should be the values. That seems more
annoying to me as a reader.
>> 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.
Why not just define the entire enum then?...
This approach seems quite common for all device emulation code.
E.g. BTSTAT_HATIMEOUT in HostBusAdapterStatus,
PVSCSI_REG_OFFSET_LAST_STS_3 in PVSCSIRegOffset, VMXNET3_CMD_UPDATE_IML
in vmxnet3.h and etc.
>> 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.
>
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.
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.
-Liran
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.
> 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.
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 ...
* See https://github.com/vmware/open-vm-tools/blob/master/open-vm-tools/lib/include/vm_vmx_type.h
* 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.
> 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.
--
MST
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
On Tue, Mar 10, 2020 at 04:46:19PM +0200, Liran Alon wrote: > There is no license issue here. It's only definitions. So it seems that in your opinion - definition names in the interface do not need a license and - it is fair to reuse them without a license for the purpose of making your compatible interface easier to use for people familiar with the original Did I get that right? -- MST
On 10/03/2020 23:16, Michael S. Tsirkin wrote: > On Tue, Mar 10, 2020 at 04:46:19PM +0200, Liran Alon wrote: >> There is no license issue here. It's only definitions. > So it seems that in your opinion > - definition names in the interface do not need a license > and > - it is fair to reuse them without a license for the purpose > of making your compatible interface easier to use for > people familiar with the original > > Did I get that right? I'm for sure not an expert on open source code licenses. You probably know this area much more than I do. But yes, this is what I would have thought. That it's not an issue to copy the enum definition. If I'm wrong and it is an issue, is declaring a new enum with new names not a license issue and can be done instead? Or am I only allowed to use hard-coded numbers, point to original code from where I deduced the number, and specify number meaning in comment? -Liran
On Tue, Mar 10, 2020 at 11:34:00PM +0200, Liran Alon wrote: > > On 10/03/2020 23:16, Michael S. Tsirkin wrote: > > On Tue, Mar 10, 2020 at 04:46:19PM +0200, Liran Alon wrote: > > > There is no license issue here. It's only definitions. > > So it seems that in your opinion > > - definition names in the interface do not need a license > > and > > - it is fair to reuse them without a license for the purpose > > of making your compatible interface easier to use for > > people familiar with the original > > > > Did I get that right? > I'm for sure not an expert on open source code licenses. You probably know > this area much more than I do. > But yes, this is what I would have thought. That it's not an issue to copy > the enum definition. I'm not a lawyer. I think attribution is important even for small things, and it was missing in v1. v2 has it but link would be better. I also think respecting author's wishes is important, and a license gives a hint as to that. -- MST
On 10/03/2020 23:49, Michael S. Tsirkin wrote: > On Tue, Mar 10, 2020 at 11:34:00PM +0200, Liran Alon wrote: >> On 10/03/2020 23:16, Michael S. Tsirkin wrote: >>> On Tue, Mar 10, 2020 at 04:46:19PM +0200, Liran Alon wrote: >>>> There is no license issue here. It's only definitions. >>> So it seems that in your opinion >>> - definition names in the interface do not need a license >>> and >>> - it is fair to reuse them without a license for the purpose >>> of making your compatible interface easier to use for >>> people familiar with the original >>> >>> Did I get that right? >> I'm for sure not an expert on open source code licenses. You probably know >> this area much more than I do. >> But yes, this is what I would have thought. That it's not an issue to copy >> the enum definition. > I'm not a lawyer. I think attribution is important even for small > things, and it was missing in v1. v2 has it but link would be better. I > also think respecting author's wishes is important, and a license gives > a hint as to that. > Oh maybe I misunderstood you. So you're saying I should just add a link to open-vm-tools git-repo for attribution? Which author's wishes you refer to? -Liran
On Tue, Mar 10, 2020 at 11:59:29PM +0200, Liran Alon wrote: > > On 10/03/2020 23:49, Michael S. Tsirkin wrote: > > On Tue, Mar 10, 2020 at 11:34:00PM +0200, Liran Alon wrote: > > > On 10/03/2020 23:16, Michael S. Tsirkin wrote: > > > > On Tue, Mar 10, 2020 at 04:46:19PM +0200, Liran Alon wrote: > > > > > There is no license issue here. It's only definitions. > > > > So it seems that in your opinion > > > > - definition names in the interface do not need a license > > > > and > > > > - it is fair to reuse them without a license for the purpose > > > > of making your compatible interface easier to use for > > > > people familiar with the original > > > > > > > > Did I get that right? > > > I'm for sure not an expert on open source code licenses. You probably know > > > this area much more than I do. > > > But yes, this is what I would have thought. That it's not an issue to copy > > > the enum definition. > > I'm not a lawyer. I think attribution is important even for small > > things, and it was missing in v1. v2 has it but link would be better. I > > also think respecting author's wishes is important, and a license gives > > a hint as to that. > > > Oh maybe I misunderstood you. > So you're saying I should just add a link to open-vm-tools git-repo for > attribution? I even posted a code snippet, feel free to reuse. > Which author's wishes you refer to? > > -Liran I really just said one should check before copying code. In this case if we don't really need to make vmport depend on GPL only code we shouldn't since original author wanted it to be copyleft. -- MST
© 2016 - 2026 Red Hat, Inc.