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

From: Liran Alon <liran.alon@oracle.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: ehabkost@redhat.com, qemu-devel@nongnu.org,
	Nikita Leshenko <nikita.leshchenko@oracle.com>,
	pbonzini@redhat.com, rth@twiddle.net
Subject: Re: [PATCH 06/14] hw/i386/vmport: Define enum for all commands
Date: Tue, 10 Mar 2020 13:54:08 +0200	[thread overview]
Message-ID: <98de576b-7a66-1e17-f404-2d25b650a2ee@oracle.com> (raw)
In-Reply-To: <20200310074457-mutt-send-email-mst@kernel.org>


On 10/03/2020 13:46, Michael S. Tsirkin wrote:
> 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.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
>>> 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.

Will do then in v2. :)

-Liran