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

["Michael S. Tsirkin" <mst@redhat.com>]

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>
> ---
>  hw/i386/vmmouse.c    | 18 ++++++------------
>  hw/i386/vmport.c     | 11 ++---------
>  include/hw/i386/pc.h | 11 ++++++++++-
>  3 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index e8e62bd96b8c..a61042fc0c5e 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -33,12 +33,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
> @@ -196,10 +190,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);
> @@ -218,7 +212,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:
> @@ -275,9 +269,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 c03f57f2f636..2ae5afc42b50 100644
> --- a/hw/i386/vmport.c
> +++ b/hw/i386/vmport.c
> @@ -30,10 +30,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
>  
>  typedef enum {
> @@ -60,12 +56,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/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.

>  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