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

[Thomas Huth]

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