Re: [Qemu-devel] [PATCH 2/2] warn about two vga cards

[Philippe Mathieu-Daudé]

On 06/27/2018 10:51 AM, Gerd Hoffmann wrote:
> On Wed, Jun 27, 2018 at 10:10:17AM -0300, Philippe Mathieu-Daudé wrote:
>> Hi Gerd,
>>
>> On 06/27/2018 08:58 AM, Gerd Hoffmann wrote:
>>> Two vga cards will listen on the same legacy (isa) ioports.  Due to this
>>> conflict only one of the two cards will work correctly in vga mode.
>>
>> The last one registered...
>>
>>> Print a warning message in that case.
>>>
>>> Signed-off-by: Gerd Hoffmann <address@hidden>
>>> ---
>>>  hw/display/vga.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/hw/display/vga.c b/hw/display/vga.c
>>> index c82e6d240a..a6c246f76b 100644
>>> --- a/hw/display/vga.c
>>> +++ b/hw/display/vga.c
>>> @@ -2273,6 +2273,7 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object 
>>> *obj,
>>>  void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>>>                MemoryRegion *address_space_io, bool init_vga_ports)
>>>  {
>>> +    static bool vgaports_registered;
>>>      MemoryRegion *vga_io_memory;
>>>      const MemoryRegionPortio *vga_ports, *vbe_ports;
>>>  
>>> @@ -2289,6 +2290,11 @@ void vga_init(VGACommonState *s, Object *obj, 
>>> MemoryRegion *address_space,
>>>                                          1);
>>>      memory_region_set_coalescing(vga_io_memory);
>>>      if (init_vga_ports) {
>>> +        if (vgaports_registered) {
>>> +            warn_report("multiple vga cards may not work as expected");
>>> +        } else {
>>> +            vgaports_registered = true;
>>> +        }
>>>          portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
>>
>> Here we leak s->vga_port_list->regions from the 1st card.
>> Can we call portio_list_destroy() in the if (vgaports_registered) block?
> 
> We don't have a pointer at hand.  Easier would be to only register
> the ports in the else block.

I was thinking of:

        if (vgaports_registered) {
            warn_report("multiple vga cards may not work as expected");
            portio_list_destroy(&s->vga_port_list);
        } else {
            vgaports_registered = true;
        }

But "only register the ports in the else block" is easier indeed.

> 
> Or throw an error in the if (vgaports_registered) block instead of
> printing a warning only.
> 
> Hmm ...

Let's see if there interested user replying to this thread...