From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34934) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ae792-000453-1f for qemu-devel@nongnu.org; Thu, 10 Mar 2016 15:25:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ae78x-0008Ts-3D for qemu-devel@nongnu.org; Thu, 10 Mar 2016 15:25:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54062) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ae78w-0008To-RF for qemu-devel@nongnu.org; Thu, 10 Mar 2016 15:25:11 -0500 References: <1457630937-28056-1-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56E1D824.9060704@redhat.com> Date: Thu, 10 Mar 2016 13:25:08 -0700 MIME-Version: 1.0 In-Reply-To: <1457630937-28056-1-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xVL1U0b4Pwqm6VbMScpusUPXn5U0jbg8p" Subject: Re: [Qemu-devel] [PATCH v1] hw: fix error reporting for missing option ROMs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Paolo Bonzini , "Michael S. Tsirkin" , Eduardo Habkost , Richard Henderson This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --xVL1U0b4Pwqm6VbMScpusUPXn5U0jbg8p Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/10/2016 10:28 AM, Daniel P. Berrange wrote: > If QEMU fails to load any of the VGA ROMs, it prints a message > to stderr and then carries on as if everything was fine, despite > the VGA interface not being functional. This extends the the > rom_add_file() method to accept a 'Error **errp' parameter. The > VGA device realizefn() impls can now pass in the errp they already > have and get errors reported as fatal problems. >=20 > Signed-off-by: Daniel P. Berrange > --- > hw/core/loader.c | 40 +++++++++++++++++++++++++--------------- > hw/display/cirrus_vga.c | 4 +++- > hw/display/vga-isa.c | 4 +++- > hw/i386/pc.c | 4 ++-- > hw/i386/pc_sysfw.c | 2 +- > hw/misc/sga.c | 4 +++- > hw/pci/pci.c | 8 ++++++-- > include/hw/loader.h | 16 +++++++++------- > 8 files changed, 52 insertions(+), 30 deletions(-) >=20 > diff --git a/hw/core/loader.c b/hw/core/loader.c > index 8e8031c..010e442 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -142,7 +142,7 @@ int load_image_targphys(const char *filename, > return -1; > } > if (size > 0) { > - rom_add_file_fixed(filename, addr, -1); > + rom_add_file_fixed(filename, addr, -1, NULL); > } Why is this one ignoring the error? Would &error_abort be better if we know it can't fail? > return size; > } > @@ -162,7 +162,7 @@ int load_image_mr(const char *filename, MemoryRegio= n *mr) > return -1; > } > if (size > 0) { > - if (rom_add_file_mr(filename, mr, -1) < 0) { > + if (rom_add_file_mr(filename, mr, -1, NULL) < 0) { > return -1; This one still detects and passes on failure, but loses the error message. I guess that's okay, as long as this patch is incrementally better somewhere else. > @@ -847,8 +849,9 @@ int rom_add_file(const char *file, const char *fw_d= ir, > =20 > fd =3D open(rom->path, O_RDONLY | O_BINARY); > if (fd =3D=3D -1) { > - fprintf(stderr, "Could not open option rom '%s': %s\n", > - rom->path, strerror(errno)); > + error_setg_errno(errp, errno, > + "Could not open option rom '%s'", > + rom->path); would error_setg_file_open() be any better here, for consistency? > +++ b/hw/i386/pc.c > @@ -1264,7 +1264,7 @@ void xen_load_linux(PCMachineState *pcms) > for (i =3D 0; i < nb_option_roms; i++) { > assert(!strcmp(option_rom[i].name, "linuxboot.bin") || > !strcmp(option_rom[i].name, "multiboot.bin")); > - rom_add_option(option_rom[i].name, option_rom[i].bootindex); > + rom_add_option(option_rom[i].name, option_rom[i].bootindex, NU= LL); Another place that blindly ignores things; should we use &error_abort? > +++ b/hw/i386/pc_sysfw.c > @@ -199,7 +199,7 @@ static void old_pc_system_rom_init(MemoryRegion *ro= m_memory, bool isapc_ram_fw) > if (!isapc_ram_fw) { > memory_region_set_readonly(bios, true); > } > - ret =3D rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);= > + ret =3D rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1, = NULL); > if (ret !=3D 0) { > bios_error: > fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_na= me); This one makes sense - you are incrementally improving the interface, and not all callers; this caller was already reporting errors and could be cleaned up in a later commit to use &err instead of fprintf(). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --xVL1U0b4Pwqm6VbMScpusUPXn5U0jbg8p Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJW4dgkAAoJEKeha0olJ0NqYIMH/2xtJlxM9co26fBhyhbNFQfi sJtg+ESJ4eISu7zAeCiTkGLGCGshL7/pZJYOge01MkmZGGNGh7WfHhne4XEOnLd9 iWBJwdrUe3r978PGrsLs4EuUlCEkTxPhV12e3OD/Pizz4CNDsEqCEUylQt/aJDGw wY1TJm0znksGQajfcYCLUIW28MACAhA7GwYA1lFItcU36VPBIbjb5wKAMgb4QG8z p1RPhXO6yekG36Zg68jqqoRV3fonBq8JkE2EMzQNzNHtQfR5AKvLh7lIn5QEdfWO 4XB7oFUlB80v96bxxCzY68DI/tZEKWOMThMnW8SBJLZFwZbYIWv2/mTm8o4UBNw= =Zh7Y -----END PGP SIGNATURE----- --xVL1U0b4Pwqm6VbMScpusUPXn5U0jbg8p--