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

[Michael S. Tsirkin]

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