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 19:58:15 +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 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







reply via email to

[Prev in Thread] Current Thread [Next in Thread]