Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

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 <address@hidden>
> > ---
> >   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