[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get() |
Date: |
Thu, 7 Jan 2016 09:47:37 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 |
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 <address@hidden>
>>> ---
>>> 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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v3 3/4] Add Error **errp for xen_pt_config_init(), (continued)
[Qemu-devel] [PATCH v3 4/4] Xen PCI passthru: convert to realize(), Cao jin, 2016/01/05
[Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get(), Cao jin, 2016/01/05
Re: [Qemu-devel] [PATCH v3 0/4] Convert to realize(), Stefano Stabellini, 2016/01/06