On Tue, Mar 10, 2020 at 01:37:40PM +0200, Liran Alon wrote:
>
> On 10/03/2020 13:23, Michael S. Tsirkin wrote:
> > On Tue, Mar 10, 2020 at 01:16:51PM +0200, Liran Alon wrote:
> > > 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.leshche...@oracle.com>
> > > > > Signed-off-by: Liran Alon <liran.a...@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
> > Well I just don't want lots of enums in pc.h
>
> This is the only one which is global, and makes sense as it's directly
> related to vmport_register() exposed API.
> Similar to how the VMPortReadFunc typedef is put in here.
>
> -Liran
So pls find another home for this stuff. Whoever touches legacy code
gets to clean it up a bit first :) Tough but that's the only stick
maintainers have to make maintainance happen.
--
MST
