Re: [PATCH v3 08/16] hw/i386/vmport: Define enum for all commands

[Philippe Mathieu-Daudé]

On 3/12/20 5:54 PM, 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 <address@hidden>
> Signed-off-by: Liran Alon <address@hidden>
> ---
>   hw/i386/vmmouse.c        | 18 ++++++------------
>   hw/i386/vmport.c         | 11 ++---------
>   include/hw/i386/vmport.h | 11 ++++++++++-

Please setup scripts/git.orderfile to ease reviews.

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>   3 files changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index 49a546fd3bb6..afb8e9e09a30 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -34,12 +34,6 @@
>   /* debug only vmmouse */
>   //#define DEBUG_VMMOUSE
>   
> -/* VMMouse Commands */
> -#define VMMOUSE_GETVERSION     10
> -#define VMMOUSE_DATA           39
> -#define VMMOUSE_STATUS         40
> -#define VMMOUSE_COMMAND                41
> -
>   #define VMMOUSE_READ_ID                       0x45414552
>   #define VMMOUSE_DISABLE                       0x000000f5
>   #define VMMOUSE_REQUEST_RELATIVE      0x4c455252
> @@ -197,10 +191,10 @@ static uint32_t vmmouse_ioport_read(void *opaque, 
> uint32_t addr)
>       command = data[2] & 0xFFFF;
>   
>       switch (command) {
> -    case VMMOUSE_STATUS:
> +    case VMPORT_CMD_VMMOUSE_STATUS:
>           data[0] = vmmouse_get_status(s);
>           break;
> -    case VMMOUSE_COMMAND:
> +    case VMPORT_CMD_VMMOUSE_COMMAND:
>           switch (data[1]) {
>           case VMMOUSE_DISABLE:
>               vmmouse_disable(s);
> @@ -219,7 +213,7 @@ static uint32_t vmmouse_ioport_read(void *opaque, uint32_t 
> addr)
>               break;
>           }
>           break;
> -    case VMMOUSE_DATA:
> +    case VMPORT_CMD_VMMOUSE_DATA:
>           vmmouse_data(s, data, data[1]);
>           break;
>       default:
> @@ -276,9 +270,9 @@ static void vmmouse_realizefn(DeviceState *dev, Error 
> **errp)
>           return;
>       }
>   
> -    vmport_register(VMMOUSE_STATUS, vmmouse_ioport_read, s);
> -    vmport_register(VMMOUSE_COMMAND, vmmouse_ioport_read, s);
> -    vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_STATUS, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_COMMAND, vmmouse_ioport_read, s);
> +    vmport_register(VMPORT_CMD_VMMOUSE_DATA, vmmouse_ioport_read, s);
>   }
>   
>   static Property vmmouse_properties[] = {
> diff --git a/hw/i386/vmport.c b/hw/i386/vmport.c
> index e9ea5fe7f765..6ab094311d6c 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -38,10 +38,6 @@
>   #include "qemu/log.h"
>   #include "trace.h"
>   
> -#define VMPORT_CMD_GETVERSION 0x0a
> -#define VMPORT_CMD_GETRAMSIZE 0x14
> -
> -#define VMPORT_ENTRIES 0x2c
>   #define VMPORT_MAGIC   0x564D5868
>   
>   /* Compatibility flags for migration */
> @@ -72,12 +68,9 @@ typedef struct VMPortState {
>   
>   static VMPortState *port_state;
>   
> -void vmport_register(unsigned char command, VMPortReadFunc *func, void *opaque)
> +void vmport_register(VMPortCommand command, VMPortReadFunc *func, void *opaque)
>   {
> -    if (command >= VMPORT_ENTRIES) {
> -        return;
> -    }
> -
> +    assert(command < VMPORT_ENTRIES);
>       trace_vmport_register(command, func, opaque);
>       port_state->func[command] = func;
>       port_state->opaque[command] = opaque;
> diff --git a/include/hw/i386/vmport.h b/include/hw/i386/vmport.h
> index f0c1e985ca08..0501ccac6ddf 100644
> --- a/include/hw/i386/vmport.h
> +++ b/include/hw/i386/vmport.h
> @@ -4,12 +4,21 @@
>   #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;
> +
>   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);
>