[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ

From: Liran Alon
Subject: Re: [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ
Date: Sun, 15 Mar 2020 02:10:41 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

On 14/03/2020 23:52, Michael S. Tsirkin wrote:
On Sat, Mar 14, 2020 at 12:44:55AM +0200, Liran Alon wrote:
On 13/03/2020 22:07, Philippe Mathieu-Daudé wrote:
On 3/12/20 5:54 PM, Liran Alon wrote:
diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
index 34cc050b1ffa..aee809521aa0 100644
--- a/include/hw/i386/vmport.h
+++ b/include/hw/i386/vmport.h
@@ -12,6 +12,7 @@ typedef enum {
       VMPORT_CMD_VMMOUSE_DATA     = 39,
+    VMPORT_CMD_GETHZ            = 45,
Can you rename to something easier to read, such _GET_FREQS_HZ or nicer?

I actually prefer to stick with names similar to open-vm-tools. i.e. Similar
to the definitions in lib/include/backdoor_def.h.
Please, do not copy without attribution. It really applies everywhere,
I commented on another enum and you fixed it there, but please
go over your code and try to generally apply the same rules.
This is not a copy of the enum as the other case you replied on.
It's just names "inspired" or "similar" to original names. They are not even the same.

This helps correlates a command in QEMU code to guest code (in
open-vm-tools) that interacts with it.
I can rename to just VMPORT_CMD_GET_HZ (Similar to what you suggested for
previous commands).
But I don't have a strong opinion on this. If you still think _GET_FREQ_HZ
is preferred, I will rename to that.


Generally I don't think a hard to read code somewhere is a good reason
to have hard to read code in QEMU, especially since it tends to
proliferate.  It seems unlikely that VMPORT_CMD_GETHZ appears verbatim
anywhere, and applying transformation rules is just too tricky. The best
way to map host code to guest code in light of coding style differences
etc is using comments. You did it in case of the type values, it
applies equally here.

Honestly, even though I used slightly different names than original open-vm-tools code, I think it's quite trivial to coorelate. Both by similar name (not same), by value and by function. That's why I don't have a strong opinion about the name. I think VMPORT_CMD_GET_HZ is sufficient, but honestly I would name it however you want. I really don't care.

I don't think any special comment is necessary here for correlation. But I don't mind putting above enum a general comment such as: /* See open-vm-tools lib/include/backdoor_def.h to match these to guest commands */


reply via email to

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