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

[Cao jin <caoj.fnst@cn.fujitsu.com>]

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.fnst@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