Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Thomas Huth <thuth@redhat.com>]

On 17/03/2022 08.40, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 16/03/2022 15.16, Markus Armbruster wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>>
>>>> On 16/03/2022 14.32, Philippe Mathieu-Daudé wrote:
>>>>> On 16/3/22 14:24, Thomas Huth wrote:
>>>>>> The vga_common_init() function currently cannot report errors to its
>>>>>> caller. But in the following patch, we'd need this possibility, so
>>>>>> let's change it to take an "Error **" as parameter for this.
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>     hw/display/ati.c            |  7 ++++++-
>>>>>>     hw/display/cirrus_vga.c     |  7 ++++++-
>>>>>>     hw/display/cirrus_vga_isa.c |  7 ++++++-
>>>>>>     hw/display/qxl.c            |  6 +++++-
>>>>>>     hw/display/vga-isa.c        |  9 ++++++++-
>>>>>>     hw/display/vga-mmio.c       |  8 +++++++-
>>>>>>     hw/display/vga-pci.c        | 15 +++++++++++++--
>>>>>>     hw/display/vga.c            |  9 +++++++--
>>>>>>     hw/display/vga_int.h        |  2 +-
>>>>>>     hw/display/virtio-vga.c     |  7 ++++++-
>>>>>>     hw/display/vmware_vga.c     |  2 +-
>>>>>>     11 files changed, 66 insertions(+), 13 deletions(-)
>>>>>
>>>>> Please setup scripts/git.orderfile :)
>>>>>
>>>>>> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
>>>>>> index 847e784ca6..3e8892df28 100644
>>>>>> --- a/hw/display/vga_int.h
>>>>>> +++ b/hw/display/vga_int.h
>>>>>> @@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
>>>>>>         return (v << 2) | (b << 1) | b;
>>>>>>     }
>>>>>> -void vga_common_init(VGACommonState *s, Object *obj);
>>>>>> +void vga_common_init(VGACommonState *s, Object *obj, Error **errp);
>>>>>
>>>>> Can we also return a boolean value? IIUC Markus recommended to check
>>>>> a boolean return value rather than Error* handle.
>>>>
>>>> Really? A very quick grep shows something different:
>>>>
>>>> $ grep -r ^void.*Error include/ | wc -l
>>>> 94
>>>> $ grep -r ^bool.*Error include/ | wc -l
>>>> 46
>>>
>>> Historical reasons.  We deviated from GLib here only to find out that
>>> the deviation leads to awkward code.  I flipped the guidance in commit
>>> e3fe3988d7 "error: Document Error API usage rules" (2020-07-10).  A lot
>>> of old code remains.
>>
>> Hmm, you should add some BiteSizeTasks to our issue tracker then to
>> get this fixed, otherwise people like me will copy-n-paste the bad
>> code examples that are all over the place!
> 
> I'm not sure the issue tracker is a good fit here.  An issue tracker
> works best when you use it to track units of work that can be completed
> in one go.  An issue then tracks progress of its unit of work.

That's why I wrote "*some* BiteSizeTasks", not "one BitSizeTask" ... of 
course you've got to break it down for unexperienced new developers first, 
e.g. by subsystem. I did that for example for the indentation clean up tasks:

  https://gitlab.com/qemu-project/qemu/-/issues/376
  https://gitlab.com/qemu-project/qemu/-/issues/371
  https://gitlab.com/qemu-project/qemu/-/issues/372

> This isn't a unit, it's more like a class of units.
> 
> I added an item to https://wiki.qemu.org/ToDo/CodeTransitions for now.

Thanks, but I doubt that this will help much - the description is really 
terse, and for anybody who is not involved in this email thread here, I 
guess it's hard for them to figure out the related parts in the 
include/qapi/error.h on their own ... so if you really want somebody else to 
work on this topic, I think you have to elaborate there a little bit (e.g. 
by giving an example in-place).

  Thomas