Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4076 bytes --]

On 01/06/2016 08:13 PM, Cao jin wrote:
> 
> 
> On 01/06/2016 11:51 PM, Eric Blake wrote:
>> On 01/05/2016 07:39 PM, Cao jin wrote:
>>> To catch the error msg. Also modify the caller
>>>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>> ---

>>> xen_host_pci_get_resource(XenHostPCIDevice *d)
>>>
>>>       rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
>>>       if (rc) {
>>> -        return rc;
>>> +        error_setg_errno(errp, errno, "snprintf err");
>>
>> Are you sure that errno is relevant?  And "snprintf err" doesn't seem to
>> be the correct message, as there is no snprintf in the line above.
>>
> 
> snprintf() is called in xen_host_pci_sysfs_path() above, and is the only
> possible error source, so I guess the errno is relevant?

Then maybe it's better to pass Error **errp through
xen_host_pci_sysfs_path(), so that the error message is closer to the
real error.  Especially since you have multiple callers, all identically
affected (each caller shouldn't have to repeat the same common
error-handling logic, if you can push it down lower in the callstack).

For that matter, the only failure of xen_host_pci_sysfs_path is to
return -ENODEV, but it does NOT set 'errno = ENODEV' on that error path,
so you most likely ARE printing the wrong errno value.

Another option is to at least reword the message to make sense locally,
as in:

if (xen_host_pci_sysfs_path(...)) {
    error_setg(errp, "failed to determine device path for %s", name);
    return;
}

> 
> Or, replace the error_setg_errno() to assert(0)? because if snprintf
> goes wrong, user seems can do nothing.

If you want to assert, then do that in xen_host_pci_sysfs_path(), not in
all callers; at which point xen_host_pci_sysfs_path() should be fixed to
return void.


>>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>>> +                            uint8_t bus, uint8_t dev, uint8_t func,
>>> +                            Error **errp)
>>>   {
>>>       unsigned int v;
>>> -    int rc = 0;
>>> +    Error *local_err = NULL;
>>
>> These days, naming the local variable 'err' is more common than
>> 'local_err'.
>>
> 
> agree. I guess maybe "local_err" is a better mnemonic for newbie. and
> when I am gradually familiar with error report, I also prefer "err".
> Actually I considered to change this name, I just don`t want to bother
> the reviewer to review it again, especially when the patch got a
> Review-by and new version just changes some names. Will fix it.

We don't mind re-reviewing a patch, but it does help if your cover
letter and/or text after the --- divider explains how v4 differs from
v3, to help us focus on the changes.

>>> +    xen_host_pci_device_get(&s->real_device,
>>> +                            s->hostaddr.domain, s->hostaddr.bus,
>>> +                            s->hostaddr.slot, s->hostaddr.function,
>>> +                            &local_err);
>>> +    if (local_err) {
>>> +        XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
>>
>> Leaks local_err.
>>
> 
> Yes, but this will be fixed with error_propagate when patch "4/4 convert
> to realize" comes, so is it ok to let it be here?

No. Every patch should be stand-alone correct, or else strongly document
why it is taking a shortcut that a later patch will clean up.  At a bare
minimum, if you don't fix the leak here, you should have a FIXME comment
both here and in the commit message; but it's probably easier to just
avoid the leak in the first place.

> 
> I originally want to make these patch independent from each other with
> the most necessary modification.

Splitting patches can be an art form.  But your comment about the patch
being independent _is_ the reason why the patch must not leak - if a
backporter uses this patch but not the rest of the series, they should
not be introducing a leak in their backport.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]