| Message ID | AANLkTimZRugosG2YUzojq4GNoGZ4GX9tLghHyWpEzmcn@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Blue Swirl <blauwirbel@gmail.com> writes: > Allow failure with vmware_vga device creation and use standard > VGA instead. > > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > --- > hw/mips_malta.c | 6 +++++- > hw/pc.c | 11 ++++++++--- > hw/vmware_vga.h | 11 +++++++++-- > 3 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/hw/mips_malta.c b/hw/mips_malta.c > index 2d3f242..930c51c 100644 > --- a/hw/mips_malta.c > +++ b/hw/mips_malta.c > @@ -957,7 +957,11 @@ void mips_malta_init (ram_addr_t ram_size, > if (cirrus_vga_enabled) { > pci_cirrus_vga_init(pci_bus); > } else if (vmsvga_enabled) { > - pci_vmsvga_init(pci_bus); > + if (!pci_vmsvga_init(pci_bus)) { > + fprintf(stderr, "Warning: vmware_vga not available," > + " using standard VGA instead\n"); > + pci_vga_init(pci_bus); > + } > } else if (std_vga_enabled) { > pci_vga_init(pci_bus); > } > diff --git a/hw/pc.c b/hw/pc.c > index 4dfdc0b..fcee09a 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -1053,10 +1053,15 @@ void pc_vga_init(PCIBus *pci_bus) > isa_cirrus_vga_init(); > } > } else if (vmsvga_enabled) { > - if (pci_bus) > - pci_vmsvga_init(pci_bus); > - else > + if (pci_bus) { > + if (!pci_vmsvga_init(pci_bus)) { > + fprintf(stderr, "Warning: vmware_vga not available," > + " using standard VGA instead\n"); I don't like "vmware_vga" here. The command line option that makes us go here is spelled "-vga vmware", and the qdev is called "vmware-svga". What about "-vga vmware is not available"? > + pci_vga_init(pci_bus); > + } > + } else { > fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__); > + } > #ifdef CONFIG_SPICE > } else if (qxl_enabled) { > if (pci_bus) > diff --git a/hw/vmware_vga.h b/hw/vmware_vga.h > index e7bcb22..5132573 100644 > --- a/hw/vmware_vga.h > +++ b/hw/vmware_vga.h > @@ -4,9 +4,16 @@ > #include "qemu-common.h" > > /* vmware_vga.c */ > -static inline void pci_vmsvga_init(PCIBus *bus) > +static inline bool pci_vmsvga_init(PCIBus *bus) > { > - pci_create_simple(bus, -1, "vmware-svga"); > + PCIDevice *dev; > + > + dev = pci_try_create(bus, -1, "vmware-svga"); > + if (!dev || qdev_init(&dev->qdev) < 0) { > + return false; > + } else { > + return true; > + } > } > > #endif Two failure modes: * pci_try_create() fails, which can happen only if no such qdev "vmware-svga" exists. * qdev_init() fails. The caller can't distinguish between the two, and assumes any failure must be the former. The assumption is actually correct, because pci_vmsvga_initfn() never fails, and thus qdev_init() never fails. Brittle. pci_vmsvga_init() is a convenience function for board setup code. Why not make it more convenient and concentrate the error handling there rather than duplicating it in each caller? Nice side effect: no need to conflate the failure modes, no need for the brittle assumption.
On Sat, Feb 12, 2011 at 6:48 PM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> Allow failure with vmware_vga device creation and use standard >> VGA instead. >> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >> --- >> hw/mips_malta.c | 6 +++++- >> hw/pc.c | 11 ++++++++--- >> hw/vmware_vga.h | 11 +++++++++-- >> 3 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/hw/mips_malta.c b/hw/mips_malta.c >> index 2d3f242..930c51c 100644 >> --- a/hw/mips_malta.c >> +++ b/hw/mips_malta.c >> @@ -957,7 +957,11 @@ void mips_malta_init (ram_addr_t ram_size, >> if (cirrus_vga_enabled) { >> pci_cirrus_vga_init(pci_bus); >> } else if (vmsvga_enabled) { >> - pci_vmsvga_init(pci_bus); >> + if (!pci_vmsvga_init(pci_bus)) { >> + fprintf(stderr, "Warning: vmware_vga not available," >> + " using standard VGA instead\n"); >> + pci_vga_init(pci_bus); >> + } >> } else if (std_vga_enabled) { >> pci_vga_init(pci_bus); >> } >> diff --git a/hw/pc.c b/hw/pc.c >> index 4dfdc0b..fcee09a 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -1053,10 +1053,15 @@ void pc_vga_init(PCIBus *pci_bus) >> isa_cirrus_vga_init(); >> } >> } else if (vmsvga_enabled) { >> - if (pci_bus) >> - pci_vmsvga_init(pci_bus); >> - else >> + if (pci_bus) { >> + if (!pci_vmsvga_init(pci_bus)) { >> + fprintf(stderr, "Warning: vmware_vga not available," >> + " using standard VGA instead\n"); > > I don't like "vmware_vga" here. The command line option that makes us > go here is spelled "-vga vmware", and the qdev is called "vmware-svga". > What about "-vga vmware is not available"? Maybe. Patches welcome. > >> + pci_vga_init(pci_bus); >> + } >> + } else { >> fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__); >> + } >> #ifdef CONFIG_SPICE >> } else if (qxl_enabled) { >> if (pci_bus) >> diff --git a/hw/vmware_vga.h b/hw/vmware_vga.h >> index e7bcb22..5132573 100644 >> --- a/hw/vmware_vga.h >> +++ b/hw/vmware_vga.h >> @@ -4,9 +4,16 @@ >> #include "qemu-common.h" >> >> /* vmware_vga.c */ >> -static inline void pci_vmsvga_init(PCIBus *bus) >> +static inline bool pci_vmsvga_init(PCIBus *bus) >> { >> - pci_create_simple(bus, -1, "vmware-svga"); >> + PCIDevice *dev; >> + >> + dev = pci_try_create(bus, -1, "vmware-svga"); >> + if (!dev || qdev_init(&dev->qdev) < 0) { >> + return false; >> + } else { >> + return true; >> + } >> } >> >> #endif > > Two failure modes: > > * pci_try_create() fails, which can happen only if no such qdev > "vmware-svga" exists. > > * qdev_init() fails. > > The caller can't distinguish between the two, and assumes any failure > must be the former. > > The assumption is actually correct, because pci_vmsvga_initfn() never > fails, and thus qdev_init() never fails. Brittle. Instead of bool, the function could return int with various error values like -ENOENT etc. Do we care enough? > pci_vmsvga_init() is a convenience function for board setup code. Why > not make it more convenient and concentrate the error handling there > rather than duplicating it in each caller? > > Nice side effect: no need to conflate the failure modes, no need for the > brittle assumption. Nice idea, how about a patch?
diff --git a/hw/mips_malta.c b/hw/mips_malta.c index 2d3f242..930c51c 100644 --- a/hw/mips_malta.c +++ b/hw/mips_malta.c @@ -957,7 +957,11 @@ void mips_malta_init (ram_addr_t ram_size, if (cirrus_vga_enabled) { pci_cirrus_vga_init(pci_bus); } else if (vmsvga_enabled) { - pci_vmsvga_init(pci_bus); + if (!pci_vmsvga_init(pci_bus)) { + fprintf(stderr, "Warning: vmware_vga not available," + " using standard VGA instead\n"); + pci_vga_init(pci_bus); + } } else if (std_vga_enabled) { pci_vga_init(pci_bus); } diff --git a/hw/pc.c b/hw/pc.c index 4dfdc0b..fcee09a 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -1053,10 +1053,15 @@ void pc_vga_init(PCIBus *pci_bus) isa_cirrus_vga_init(); } } else if (vmsvga_enabled) { - if (pci_bus) - pci_vmsvga_init(pci_bus); - else + if (pci_bus) { + if (!pci_vmsvga_init(pci_bus)) { + fprintf(stderr, "Warning: vmware_vga not available," + " using standard VGA instead\n"); + pci_vga_init(pci_bus); + } + } else { fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__); + } #ifdef CONFIG_SPICE } else if (qxl_enabled) { if (pci_bus) diff --git a/hw/vmware_vga.h b/hw/vmware_vga.h index e7bcb22..5132573 100644 --- a/hw/vmware_vga.h +++ b/hw/vmware_vga.h @@ -4,9 +4,16 @@ #include "qemu-common.h" /* vmware_vga.c */ -static inline void pci_vmsvga_init(PCIBus *bus) +static inline bool pci_vmsvga_init(PCIBus *bus) { - pci_create_simple(bus, -1, "vmware-svga"); + PCIDevice *dev; + + dev = pci_try_create(bus, -1, "vmware-svga"); + if (!dev || qdev_init(&dev->qdev) < 0) { + return false; + } else { + return true; + } } #endif
Allow failure with vmware_vga device creation and use standard VGA instead. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- hw/mips_malta.c | 6 +++++- hw/pc.c | 11 ++++++++--- hw/vmware_vga.h | 11 +++++++++-- 3 files changed, 22 insertions(+), 6 deletions(-)