[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: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ |
Date: |
Sat, 14 Mar 2020 17:52:34 -0400 |
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_VMMOUSE_STATUS = 40,
> > > VMPORT_CMD_VMMOUSE_COMMAND = 41,
> > > + 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 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.
>
> -Liran
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.
--
MST
- [PATCH v3 12/16] hw/i386/vmport: Add support for CMD_GET_VCPU_INFO, (continued)
- [PATCH v3 12/16] hw/i386/vmport: Add support for CMD_GET_VCPU_INFO, Liran Alon, 2020/03/12
- [PATCH v3 13/16] hw/i386/vmport: Allow x2apic without IR, Liran Alon, 2020/03/12
- [PATCH v3 14/16] i386/cpu: Store LAPIC bus frequency in CPU structure, Liran Alon, 2020/03/12
- [PATCH v3 16/16] hw/i386/vmport: Assert vmport initialized before registering commands, Liran Alon, 2020/03/12
- [PATCH v3 15/16] hw/i386/vmport: Add support for CMD_GETHZ, Liran Alon, 2020/03/12