From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41059) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aAcZe-00015t-JA for qemu-devel@nongnu.org; Sun, 20 Dec 2015 06:54:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aAcZb-0001c6-Cs for qemu-devel@nongnu.org; Sun, 20 Dec 2015 06:54:50 -0500 Received: from [59.151.112.132] (port=63418 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aAcZY-0001aW-GX for qemu-devel@nongnu.org; Sun, 20 Dec 2015 06:54:47 -0500 References: <1450436632-23980-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1450436632-23980-3-git-send-email-caoj.fnst@cn.fujitsu.com> <20151218183700.GZ3774@thinpad.lan.raisama.net> From: Cao jin Message-ID: <56769785.2080305@cn.fujitsu.com> Date: Sun, 20 Dec 2015 19:56:53 +0800 MIME-Version: 1.0 In-Reply-To: <20151218183700.GZ3774@thinpad.lan.raisama.net> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: mst@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, leon.alrae@imgtec.com, aurelien@aurel32.net, rth@twiddle.net On 12/19/2015 02:37 AM, Eduardo Habkost wrote: > On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote: >> Signed-off-by: Cao jin >> --- >> hw/pci-host/piix.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c >> index 715208b..e3840f0 100644 >> --- a/hw/pci-host/piix.c >> +++ b/hw/pci-host/piix.c >> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = { >> {0xa8, 4}, /* SNB: base of GTT stolen memory */ >> }; >> >> -static int host_pci_config_read(int pos, int len, uint32_t val) >> +static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp) > > You don't need the return value anymore, if you report errors > through the errp parameter. The function can be void, now. Ok, will modify that in next version, as many people mentioned in the other patch... > >> { >> char path[PATH_MAX]; >> int config_fd; >> @@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, uint32_t val) >> int ret = 0; >> >> if (rc >= size || rc < 0) { >> + error_setg(errp, "No such device"); >> return -ENODEV; >> } >> >> config_fd = open(path, O_RDWR); >> if (config_fd < 0) { >> + error_setg(errp, "No such device"); >> return -ENODEV; >> } >> >> if (lseek(config_fd, pos, SEEK_SET) != pos) { >> + error_setg(errp, "lseek err: %s", strerror(errno)); > > What about error_setg_errno()? Ok, I am not conscious that there exist error_setg_errno() :p > >> ret = -errno; >> goto out; >> } >> @@ -789,13 +792,14 @@ static int host_pci_config_read(int pos, int len, uint32_t val) >> } while (rc < 0 && (errno == EINTR || errno == EAGAIN)); >> if (rc != len) { >> ret = -errno; >> + error_setg(errp, "read err: %s", strerror(errno)); > > Same here. OK. > >> } >> out: >> close(config_fd); >> return ret; >> } >> >> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev) >> +static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp) >> { >> uint32_t val = 0; >> int rc, i, num; >> @@ -805,14 +809,12 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev) >> for (i = 0; i < num; i++) { >> pos = igd_host_bridge_infos[i].offset; >> len = igd_host_bridge_infos[i].len; >> - rc = host_pci_config_read(pos, len, val); >> + rc = host_pci_config_read(pos, len, val, errp); >> if (rc) { > > The usual pattern for error checking and propagation is: > > Error *err; > host_pci_config_read(pos, len, val, &err); > if (err) { > error_propagate(errp, local_err); > return; > } > Yup. I see >> - return -ENODEV; >> + return; >> } >> pci_default_write_config(pci_dev, pos, val, len); >> } >> - >> - return 0; >> } >> >> static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data) >> @@ -820,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data) >> DeviceClass *dc = DEVICE_CLASS(klass); >> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> >> - k->init = igd_pt_i440fx_initfn; >> + k->realize = igd_pt_i440fx_realize; >> dc->desc = "IGD Passthrough Host bridge"; >> } >> >> -- >> 2.1.0 >> >> >> > -- Yours Sincerely, Cao Jin