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

[Markus Armbruster]

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.

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.