Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
On 01/11/2016 10:32 PM, Markus Armbruster wrote: Cao jin writes: [...] Ok...will change the error msg to strerror(errno) There's also error_setg_file_open(), not sure it fits here, but check it out. Thanks for reminding. Gerd Hoffmann has whole patchset(http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00245.html) which covers this, so maybe this doesn`t need to be updated. -- Yours Sincerely, Cao jin
Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
Cao jin writes: > On 12/19/2015 05:18 AM, Markus Armbruster wrote: >> One short remark in addition to Eduardo's review. >> >> Eduardo Habkost writes: >> > [...] config_fd = open(path, O_RDWR); if (config_fd < 0) { +error_setg(errp, "No such device"); return -ENODEV; } >> >> Can we come up with nicer error messages? >> > > Ok...will change the error msg to strerror(errno) There's also error_setg_file_open(), not sure it fits here, but check it out.
Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
On 12/19/2015 05:18 AM, Markus Armbruster wrote: One short remark in addition to Eduardo's review. Eduardo Habkost writes: [...] config_fd = open(path, O_RDWR); if (config_fd < 0) { +error_setg(errp, "No such device"); return -ENODEV; } Can we come up with nicer error messages? Ok...will change the error msg to strerror(errno) [...] . -- Yours Sincerely, Cao Jin
Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
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
Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
One short remark in addition to Eduardo's review. Eduardo Habkost writes: > 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. > >> { >> 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; >> } Can we come up with nicer error messages? [...]
Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
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. > { > 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()? > 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. > } > 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; } > -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 > > > -- Eduardo
[Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
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) { 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)); 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)); } 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) { -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