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

[Philippe Mathieu-Daudé]

On 3/13/20 8:59 PM, Philippe Mathieu-Daudé wrote:
> 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,

Can we rename as VMPORT_CMD_GET_VERSION which is easier to read?

> > +    VMPORT_CMD_GETRAMSIZE       = 20,

Ditto _GET_RAM_SIZE?

> > +    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);