| Message ID | 20220316132402.1190346-3-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix crash when adding a second ISA VGA device | expand |
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.
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 I also can't see that recommendation in docs/devel/style.rst. I think you either got that wrong, or the coding style needs an update first. Thomas
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. > I also can't see that recommendation in docs/devel/style.rst. I think you > either got that wrong, or the coding style needs an update first. It's in include/qapi/error.h: /* * Error reporting system loosely patterned after Glib's GError. * * = Rules = [...] * - Whenever practical, also return a value that indicates success / * failure. This can make the error checking more concise, and can * avoid useless error object creation and destruction. Note that * we still have many functions returning void. We recommend * • bool-valued functions return true on success / false on failure, * • pointer-valued functions return non-null / null pointer, and * • integer-valued functions return non-negative / negative. [...] * Call a function, receive an error from it, and pass it to the caller * - when the function returns a value that indicates failure, say * false: * if (!foo(arg, errp)) { * handle the error... * } * - when it does not, say because it is a void function: * ERRP_GUARD(); * foo(arg, errp); * if (*errp) { * handle the error... * } * More on ERRP_GUARD() below. [...] * Receive an error, and handle it locally * - when the function returns a value that indicates failure, say * false: * Error *err = NULL; * if (!foo(arg, &err)) { * handle the error... * } * - when it does not, say because it is a void function: * Error *err = NULL; * foo(arg, &err); * if (err) { * handle the error... * } Note that error.h's big comment I abbreviated here has some 200 non-blank lines. It's too long and detailed for style.rst (which has some 500 non-blank lines). Instead style.rst points to error.h under QEMU Specific Idioms / Error handling and reporting / Propagating errors.
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! Thomas
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.
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
Thomas Huth <thuth@redhat.com> writes: > 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> [...] >>>> 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 Okay, I get you now. When I flipped the guidance, I also updated a substantial amount of code to honor the rule: Subject: [PATCH v4 00/45] Less clumsy error checking Date: Tue, 7 Jul 2020 18:05:28 +0200 [...] This series adopts the GError rule (in writing), and updates a substantial amount of code to honor the rule. Cuts the number of error_propagate() calls nearly by half. The diffstat speaks for itself. https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg02580.html The "substantial amount of code" were a few subsystems with many, many callers. What remains, as far as I can tell, is many functions with few callers each. If I run into a bite-sized conversion task I'd like a volunteer to tackle, I should file an issue for it. But collecting the everything to be converted, then partitioning it into sane parts (*many* parts), then creating an issue for each, is not going to happen. Nobody got time for that. >> 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). "as explained in include/qapi/error.h" points to the full explanation, which includes examples.
diff --git a/hw/display/ati.c b/hw/display/ati.c index 31f22754dc..d2def7a4d3 100644 --- a/hw/display/ati.c +++ b/hw/display/ati.c @@ -926,6 +926,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp) { ATIVGAState *s = ATI_VGA(dev); VGACommonState *vga = &s->vga; + Error *local_err = NULL; if (s->model) { int i; @@ -955,7 +956,11 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp) } /* init vga bits */ - vga_common_init(vga, OBJECT(s)); + vga_common_init(vga, OBJECT(s), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } vga_init(vga, OBJECT(s), pci_address_space(dev), pci_address_space_io(dev), true); vga->con = graphic_console_init(DEVICE(s), 0, s->vga.hw_ops, &s->vga); diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index 7da1be3e12..037f842322 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -2944,6 +2944,7 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp) CirrusVGAState *s = &d->cirrus_vga; PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); int16_t device_id = pc->device_id; + Error *local_err = NULL; /* * Follow real hardware, cirrus card emulated has 4 MB video memory. @@ -2956,7 +2957,11 @@ static void pci_cirrus_vga_realize(PCIDevice *dev, Error **errp) return; } /* setup VGA */ - vga_common_init(&s->vga, OBJECT(dev)); + vga_common_init(&s->vga, OBJECT(dev), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } cirrus_init_common(s, OBJECT(dev), device_id, 1, pci_address_space(dev), pci_address_space_io(dev)); s->vga.con = graphic_console_init(DEVICE(dev), 0, s->vga.hw_ops, &s->vga); diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c index 4f6fb1af3b..8ac017d0ce 100644 --- a/hw/display/cirrus_vga_isa.c +++ b/hw/display/cirrus_vga_isa.c @@ -46,6 +46,7 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp) ISADevice *isadev = ISA_DEVICE(dev); ISACirrusVGAState *d = ISA_CIRRUS_VGA(dev); VGACommonState *s = &d->cirrus_vga.vga; + Error *local_err = NULL; /* follow real hardware, cirrus card emulated has 4 MB video memory. Also accept 8 MB/16 MB for backward compatibility. */ @@ -56,7 +57,11 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp) return; } s->global_vmstate = true; - vga_common_init(s, OBJECT(dev)); + vga_common_init(s, OBJECT(dev), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } cirrus_init_common(&d->cirrus_vga, OBJECT(dev), CIRRUS_ID_CLGD5430, 0, isa_address_space(isadev), isa_address_space_io(isadev)); diff --git a/hw/display/qxl.c b/hw/display/qxl.c index 1f9ad31943..adbdbcaeb6 100644 --- a/hw/display/qxl.c +++ b/hw/display/qxl.c @@ -2215,7 +2215,11 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp) qxl_init_ramsize(qxl); vga->vbe_size = qxl->vgamem_size; vga->vram_size_mb = qxl->vga.vram_size / MiB; - vga_common_init(vga, OBJECT(dev)); + vga_common_init(vga, OBJECT(dev), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } vga_init(vga, OBJECT(dev), pci_address_space(dev), pci_address_space_io(dev), false); portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list, diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c index 90851e730b..ba0f679a2d 100644 --- a/hw/display/vga-isa.c +++ b/hw/display/vga-isa.c @@ -25,6 +25,7 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/isa/isa.h" #include "vga_int.h" #include "ui/pixel_ops.h" @@ -60,9 +61,15 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp) VGACommonState *s = &d->state; MemoryRegion *vga_io_memory; const MemoryRegionPortio *vga_ports, *vbe_ports; + Error *local_err = NULL; s->global_vmstate = true; - vga_common_init(s, OBJECT(dev)); + vga_common_init(s, OBJECT(dev), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + s->legacy_address_space = isa_address_space(isadev); vga_io_memory = vga_init_io(s, OBJECT(dev), &vga_ports, &vbe_ports); isa_register_portio_list(isadev, &d->portio_vga, diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c index 4969368081..2c1b7d9973 100644 --- a/hw/display/vga-mmio.c +++ b/hw/display/vga-mmio.c @@ -88,6 +88,7 @@ static void vga_mmio_realizefn(DeviceState *dev, Error **errp) { VGAMmioState *s = VGA_MMIO(dev); SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + Error *local_err = NULL; memory_region_init_io(&s->iomem, OBJECT(dev), &vga_mm_ctrl_ops, s, "vga-mmio", 0x100000); @@ -102,7 +103,12 @@ static void vga_mmio_realizefn(DeviceState *dev, Error **errp) s->vga.bank_offset = 0; s->vga.global_vmstate = true; - vga_common_init(&s->vga, OBJECT(dev)); + vga_common_init(&s->vga, OBJECT(dev), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + sysbus_init_mmio(sbd, &s->vga.vram); s->vga.con = graphic_console_init(dev, 0, s->vga.hw_ops, &s->vga); } diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c index 62fb5c38c1..0ce159fc12 100644 --- a/hw/display/vga-pci.c +++ b/hw/display/vga-pci.c @@ -25,6 +25,7 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/pci/pci.h" #include "hw/qdev-properties.h" #include "migration/vmstate.h" @@ -235,11 +236,16 @@ static void pci_std_vga_realize(PCIDevice *dev, Error **errp) { PCIVGAState *d = PCI_VGA(dev); VGACommonState *s = &d->vga; + Error *local_err = NULL; bool qext = false; bool edid = false; /* vga + console init */ - vga_common_init(s, OBJECT(dev)); + vga_common_init(s, OBJECT(dev), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } vga_init(s, OBJECT(dev), pci_address_space(dev), pci_address_space_io(dev), true); @@ -271,11 +277,16 @@ static void pci_secondary_vga_realize(PCIDevice *dev, Error **errp) { PCIVGAState *d = PCI_VGA(dev); VGACommonState *s = &d->vga; + Error *local_err = NULL; bool qext = false; bool edid = false; /* vga + console init */ - vga_common_init(s, OBJECT(dev)); + vga_common_init(s, OBJECT(dev), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } s->con = graphic_console_init(DEVICE(dev), 0, s->hw_ops, s); /* mmio bar */ diff --git a/hw/display/vga.c b/hw/display/vga.c index 9d1f66af40..7fc6ab7e4f 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -2168,9 +2168,10 @@ static inline uint32_t uint_clamp(uint32_t val, uint32_t vmin, uint32_t vmax) return val; } -void vga_common_init(VGACommonState *s, Object *obj) +void vga_common_init(VGACommonState *s, Object *obj, Error **errp) { int i, j, v, b; + Error *local_err = NULL; for(i = 0;i < 256; i++) { v = 0; @@ -2206,7 +2207,11 @@ void vga_common_init(VGACommonState *s, Object *obj) s->is_vbe_vmstate = 1; memory_region_init_ram_nomigrate(&s->vram, obj, "vga.vram", s->vram_size, - &error_fatal); + &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } vmstate_register_ram(&s->vram, s->global_vmstate ? NULL : DEVICE(obj)); xen_register_framebuffer(&s->vram); s->vram_ptr = memory_region_get_ram_ptr(&s->vram); 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); void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space, MemoryRegion *address_space_io, bool init_vga_ports); MemoryRegion *vga_init_io(VGACommonState *s, Object *obj, diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c index 5a2f7a4540..4276c49b93 100644 --- a/hw/display/virtio-vga.c +++ b/hw/display/virtio-vga.c @@ -103,12 +103,17 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp) VirtIOVGABase *vvga = VIRTIO_VGA_BASE(vpci_dev); VirtIOGPUBase *g = vvga->vgpu; VGACommonState *vga = &vvga->vga; + Error *local_err = NULL; uint32_t offset; int i; /* init vga compat bits */ vga->vram_size_mb = 8; - vga_common_init(vga, OBJECT(vpci_dev)); + vga_common_init(vga, OBJECT(vpci_dev), &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } vga_init(vga, OBJECT(vpci_dev), pci_address_space(&vpci_dev->pci_dev), pci_address_space_io(&vpci_dev->pci_dev), true); pci_register_bar(&vpci_dev->pci_dev, 0, diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c index 0cc43a1f15..98c83474ad 100644 --- a/hw/display/vmware_vga.c +++ b/hw/display/vmware_vga.c @@ -1254,7 +1254,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s, &error_fatal); s->fifo_ptr = memory_region_get_ram_ptr(&s->fifo_ram); - vga_common_init(&s->vga, OBJECT(dev)); + vga_common_init(&s->vga, OBJECT(dev), &error_fatal); vga_init(&s->vga, OBJECT(dev), address_space, io, true); vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga); s->new_depth = 32;
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(-)