On 01/09/2016 06:50 AM, Eric Blake wrote:
On 01/08/2016 01:37 AM, Cao jin wrote:
To catch the error msg. Also modify the caller
Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
---
hw/xen/xen-host-pci-device.c | 134 ++++++++++++++++++++++---------------------
hw/xen/xen-host-pci-device.h | 5 +-
hw/xen/xen_pt.c | 13 +++--
3 files changed, 81 insertions(+), 71 deletions(-)
@@ -40,16 +40,16 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice
*d,
d->domain, d->bus, d->dev, d->func, name);
if (rc >= size || rc < 0) {
- /* The output is truncated, or some other error was encountered */
- return -ENODEV;
+ /* The output is truncated, or some other error was encountered.
+ * Assert here since user can do nothing in case of failure */
+ assert(0);
}
Might be shorter to drop the 'if' block, and just write:
assert(rc >= 0 && rc < size);
where you then don't need a comment, because the body of the assert() is
then more specific on the caller's responsibility for passing in a
decent size argument.
Yes, seems better
buf[rc] = 0;
rc = qemu_strtoul(buf, &endptr, base, &value);
Do you still need a local 'value' variable, or can you just reuse pvalue
here?
if (!rc) {
*pvalue = value;
+ } else if (rc == -EINVAL) {
+ error_setg(errp, "strtoul: Invalid argument");
+ } else {
+ error_setg_errno(errp, errno, "strtoul err");
Still not quite right - you are not guaranteed that 'errno' is sane
after qemu_strtoul(), only that -rc is sane. And feels repetitive.
Better might be:
rc = qemu_strtoul(buf, &endptr, base, pvalue);
if (rc) {
error_setg_errno(errp, -rc, "failed to parse value '%s'", buf);
}
yes, shorter and better.
But you said before: the only correct way to safely check errno after
strtol() is to first prime it to 0 prior to calling strtol. Seems
qemu_strtoul() did it as you said(first prime it to 0), so the errno is
sane? Then I guess my patch can achieve the same result as yours?
because return value of qemu_strtoul() is as following:
success: 0
failure: -EINVAL or -errno
[...]
- if (rc) {
- XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc);
+ xen_host_pci_device_get(&s->real_device,
+ s->hostaddr.domain, s->hostaddr.bus,
+ s->hostaddr.slot, s->hostaddr.function,
+ &err);
+ if (err) {
+ error_append_hint(&err, "Failed to \"open\" the real pci device");
Markus may have an opinion on whether his new error_prepend code is a
better fit (error_append_hint lists _after_ the original failure, but it
sounds like you want "failed to open the real pci device: low level
details").
But looks like you're getting closer.
greped the code, seems error_prepend doesn`t exist in upstream? So I
guess maybe we can use append hint for now?
--
Yours Sincerely,
Cao jin