Hi,
On 12/09/2016 18:17, Alex Williamson wrote:
> On Mon, 12 Sep 2016 16:00:18 +0200
> Auger Eric <eric.au...@redhat.com> wrote:
>
>> Hi Markus,
>>
>> On 12/09/2016 14:45, Markus Armbruster wrote:
>>> Eric Auger <eric.au...@redhat.com> writes:
>>>
>>>> This patch converts VFIO PCI to realize function.
>>>>
>>>> Also original initfn errors now are propagated using QEMU
>>>> error objects. All errors are formatted with the same pattern:
>>>> "vfio: %s: the error description"
>>>
>>> Example:
>>>
>>> $ upstream-qemu -device vfio-pci
>>> upstream-qemu: -device vfio-pci: vfio: 0000:00:00.0: no iommu_group found:
>>> Unknown error -2
>>>
>>> Two remarks:
>>>
>>> * "Unknown error -2" should be "No such file or directory". See below.
>> Hum. I noticed that but I didn't have the presence of mind to get it was
>> due to -errno!
>>>
>>> * Five colons, not counting the ones in the PCI address. Do we need the
>>> "vfio: 0000:00:00.0:" part? If yes, could we find a nicer way to
>>> print it? Up to you.
>> Well we have quite a lot of traces with such format, hence my choice.
>> Alex do you want to change this?
>
> Well, we need to identify the component with the error, it's not
> uncommon to have multiple assigned devices. The PCI address is just
> the basename in vfio code, it might also be the name of a device node
> in sysfs, such as a uuid of an mdev devices. AFAIK we cannot count on
> a id and even if we could libvirt assigns them based on order in the
> xml, making them a bit magic. Maybe I'm not understanding the
> question. Regarding trace vs error message, I expect that it's going
> to be a more advanced user/developer enabling tracing, error reports
> should try a little harder to be comprehensible to an average user.
On my side I would be inclined to keep the "vfio: BDF" prefix. Maybe the
PCI domain may be omitted?
>
>>>
>>> Always, always, always test your error messages :)
>>>
>>>> Subsequent patches will pass the error objects to
>>>> - vfio_populate_device,
>>>> - vfio_msix_early_setup.
>>>>
>>>> At this point if those functions fail let's just report an error
>>>> at realize level.
>>>>
>>>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>>>> ---
>>>> hw/vfio/pci.c | 81
>>>> ++++++++++++++++++++++++++++------------------------
>>>> hw/vfio/trace-events | 2 +-
>>>> 2 files changed, 44 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index 7bfa17c..ae1967c 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -2485,7 +2485,7 @@ static void
>>>> vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>>> vdev->req_enabled = false;
>>>> }
>>>>
>>>> -static int vfio_initfn(PCIDevice *pdev)
>>>> +static void vfio_realize(PCIDevice *pdev, Error **errp)
>>>> {
>>>> VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>>>> VFIODevice *vbasedev_iter;
>>>> @@ -2504,9 +2504,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>>> }
>>>>
>>>> if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
>>>> - error_report("vfio: error: no such host device: %s",
>>>> - vdev->vbasedev.sysfsdev);
>>>> - return -errno;
>>>> + error_setg_errno(errp, -errno, "vfio: error: no such host device:
>>>> %s",
>>>
>>> error_setg_errno() takes a *positive* errno. Same elsewhere.
>> OK thanks!
>>>
>>>> + vdev->vbasedev.sysfsdev);
>>>> + return;
>>>> }
>>>>
>>>> vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
>>>> @@ -2518,45 +2518,46 @@ static int vfio_initfn(PCIDevice *pdev)
>>>> g_free(tmp);
>>>>
>>>> if (len <= 0 || len >= sizeof(group_path)) {
>>>> - error_report("vfio: error no iommu_group for device");
>>>> - return len < 0 ? -errno : -ENAMETOOLONG;
>>>> + error_setg_errno(errp, len < 0 ? -errno : -ENAMETOOLONG,
>>>> + "no iommu_group found");
>>>> + goto error;
>>>> }
>>>>
>>>> group_path[len] = 0;
>>>>
>>>> group_name = basename(group_path);
>>>> if (sscanf(group_name, "%d", &groupid) != 1) {
>>>> - error_report("vfio: error reading %s: %m", group_path);
>>>> - return -errno;
>>>> + error_setg_errno(errp, -errno, "failed to read %s", group_path);
>>>> + goto error;
>>>> }
>>>>
>>>> - trace_vfio_initfn(vdev->vbasedev.name, groupid);
>>>> + trace_vfio_realize(vdev->vbasedev.name, groupid);
>>>>
>>>> group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
>>>> if (!group) {
>>>> - error_report("vfio: failed to get group %d", groupid);
>>>> - return -ENOENT;
>>>> + error_setg_errno(errp, -ENOENT, "failed to get group %d",
>>>> groupid);
>>>> + goto error;
>>>
>>> I understand this is a mechanical conversion, but are you sure the ": No
>>> such file or directory" suffix we get from passing ENOENT buys us
>>> anything? More of the same below.
>> At that stage I prefered to stick to the original messages since Alex &
>> VFIO users may have their habits.
>
> ENOENT may be a relic from previous conversions. In general I have no
> attachment to any of our error messages. I might pay more attention to
> tracing since I definitely don't want to lose functionality there, but
> for errors, so long as it's unique and descriptive, please update as
> you see fit. You can always use google to see how common a particular
> error is and whether significantly rewording it could further confuse
> or help users. Thanks,
OK in that case I will try to improve whenever it makes sense.
Thanks
Eric
>
> Alex
>
