Re: [PATCH 06/14] hw/i386/vmport: Define enum for all commands

[Liran Alon <liran.alon@oracle.com>]

On 10/03/2020 11:28, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 01:54:03AM +0200, Liran Alon wrote:
>> No functional change.
>>
>> Defining an enum for all VMPort commands have the following advantages:
>> * It gets rid of the error-prone requirement to update VMPORT_ENTRIES
>> when new VMPort commands are added to QEMU.
>> * It makes it clear to know by looking at one place at the source, what
>> are all the VMPort commands supported by QEMU.
>>
>> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index d5ac76d54e1f..7f15a01137b1 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -138,12 +138,21 @@ GSIState *pc_gsi_create(qemu_irq **irqs, bool pci_enabled);
>>   #define TYPE_VMPORT "vmport"
>>   typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>>   
>> +typedef enum {
>> +    VMPORT_CMD_GETVERSION       = 10,
>> +    VMPORT_CMD_GETRAMSIZE       = 20,
>> +    VMPORT_CMD_VMMOUSE_DATA     = 39,
>> +    VMPORT_CMD_VMMOUSE_STATUS   = 40,
>> +    VMPORT_CMD_VMMOUSE_COMMAND  = 41,
>> +    VMPORT_ENTRIES
>> +} VMPortCommand;
>> +
> Please don't, let's leave pc.h alone. If you must add a new header for
> vmport/vmmouse and put this stuff there.

As you can see, pc.h already contains definitions which are specific to 
vmport. E.g. TYPE_VMPORT, VMPortReadFunc(), vmport_register(), 
vmmouse_get_data(), vmmouse_set_data(). Adding this enum is not what 
makes the difference.
It is possible to create a new vmport.h header file but it's not really 
related to this patch. It's just general refactoring. I can do that in 
v2 if you think it's appropriate.

-Liran

>
>>   static inline void vmport_init(ISABus *bus)
>>   {
>>       isa_create_simple(bus, TYPE_VMPORT);
>>   }
>>   
>> -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque);
>> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque);
>>   void vmmouse_get_data(uint32_t *data);
>>   void vmmouse_set_data(const uint32_t *data);
>>   
>> -- 
>> 2.20.1