Re: [Qemu-devel] [PATCH v2 02/12] vfio/pci: Pass an error object to vfio_populate_device
Markus,
On 22/09/2016 17:49, Markus Armbruster wrote:
> Eric Auger writes:
>
>> Pass an error object to prepare for migration to VFIO-PCI realize.
>> The returned value will be removed later on.
>>
>> The case where error recovery cannot be enabled is not converted into
>> an error object but directly reported through error_report, as before.
>> Populating an error instead would cause the future realize function to
>> fail, which is not wanted.
>>
>> Signed-off-by: Eric Auger
>>
>> ---
>>
>> v1 -> v2:
>> - this patch now comes before the actual realize migration
>> - do not handle the error recovery issue as an error
>> ---
>> hw/vfio/pci.c | 26 +-
>> 1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ca0293d..9fe970a 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2199,7 +2199,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
>> return 0;
>> }
>>
>> -static int vfio_populate_device(VFIOPCIDevice *vdev)
>> +static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>> {
>> VFIODevice *vbasedev = &vdev->vbasedev;
>> struct vfio_region_info *reg_info;
>> @@ -2208,18 +2208,18 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>
>> /* Sanity check device */
>> if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
>> -error_report("vfio: Um, this isn't a PCI device");
>> +error_setg(errp, "this isn't a PCI device");
>> goto error;
>> }
>>
>> if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
>> -error_report("vfio: unexpected number of io regions %u",
>> - vbasedev->num_regions);
>> +error_setg(errp, "unexpected number of io regions %u",
>> + vbasedev->num_regions);
>> goto error;
>> }
>>
>> if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
>> -error_report("vfio: unexpected number of irqs %u",
>> vbasedev->num_irqs);
>> +error_setg(errp, "unexpected number of irqs %u",
>> vbasedev->num_irqs);
>> goto error;
>> }
>>
>> @@ -2231,7 +2231,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>> g_free(name);
>>
>> if (ret) {
>> -error_report("vfio: Error getting region %d info: %m", i);
>> +error_setg_errno(errp, -ret, "failed to get region %d info", i);
>> goto error;
>> }
>>
>> @@ -2241,7 +2241,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>> ret = vfio_get_region_info(vbasedev,
>> VFIO_PCI_CONFIG_REGION_INDEX, ®_info);
>> if (ret) {
>> -error_report("vfio: Error getting config info: %m");
>> +error_setg_errno(errp, -ret, "failed to get config info");
>> goto error;
>> }
>>
>> @@ -2261,8 +2261,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>> if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
>> ret = vfio_populate_vga(vdev);
>> if (ret) {
>> -error_report(
>> -"vfio: Device does not support requested feature x-vga");
>> +error_setg_errno(errp, -ret,
>> +"device does not support requested feature x-vga");
>> goto error;
>> }
>> }
>
> What about the error_report() in vfio_populate_vga()? Should the
> vfio_populate_vga() take an Error * argument?
Sure
Thanks
Eric
>
>> @@ -2277,7 +2277,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>> } else if (irq_info.count == 1) {
>> vdev->pci_aer = true;
>> } else {
>> -error_report("vfio: %s "
>> +error_report(WARN_PREFIX
>> "Could not enable error recovery for the device",
>> vbasedev->name);
>> }
>> @@ -2561,9 +2561,9 @@ static int vfio_initfn(PCIDevice *pdev)
>> goto error;
>> }
>>
>> -ret = vfio_populate_device(vdev);
>> -if (ret) {
>> -return ret;
>> +ret = vfio_populate_device(vdev, &err);
>> +if (err) {
>> +goto error;
>> }
>>
>> /* Get a copy of config space */
>
Re: [Qemu-devel] [PATCH v2 02/12] vfio/pci: Pass an error object to vfio_populate_device
Eric Auger writes:
> Pass an error object to prepare for migration to VFIO-PCI realize.
> The returned value will be removed later on.
>
> The case where error recovery cannot be enabled is not converted into
> an error object but directly reported through error_report, as before.
> Populating an error instead would cause the future realize function to
> fail, which is not wanted.
>
> Signed-off-by: Eric Auger
>
> ---
>
> v1 -> v2:
> - this patch now comes before the actual realize migration
> - do not handle the error recovery issue as an error
> ---
> hw/vfio/pci.c | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ca0293d..9fe970a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2199,7 +2199,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
> return 0;
> }
>
> -static int vfio_populate_device(VFIOPCIDevice *vdev)
> +static int vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> {
> VFIODevice *vbasedev = &vdev->vbasedev;
> struct vfio_region_info *reg_info;
> @@ -2208,18 +2208,18 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>
> /* Sanity check device */
> if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
> -error_report("vfio: Um, this isn't a PCI device");
> +error_setg(errp, "this isn't a PCI device");
> goto error;
> }
>
> if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
> -error_report("vfio: unexpected number of io regions %u",
> - vbasedev->num_regions);
> +error_setg(errp, "unexpected number of io regions %u",
> + vbasedev->num_regions);
> goto error;
> }
>
> if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
> -error_report("vfio: unexpected number of irqs %u",
> vbasedev->num_irqs);
> +error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs);
> goto error;
> }
>
> @@ -2231,7 +2231,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
> g_free(name);
>
> if (ret) {
> -error_report("vfio: Error getting region %d info: %m", i);
> +error_setg_errno(errp, -ret, "failed to get region %d info", i);
> goto error;
> }
>
> @@ -2241,7 +2241,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
> ret = vfio_get_region_info(vbasedev,
> VFIO_PCI_CONFIG_REGION_INDEX, ®_info);
> if (ret) {
> -error_report("vfio: Error getting config info: %m");
> +error_setg_errno(errp, -ret, "failed to get config info");
> goto error;
> }
>
> @@ -2261,8 +2261,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
> if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
> ret = vfio_populate_vga(vdev);
> if (ret) {
> -error_report(
> -"vfio: Device does not support requested feature x-vga");
> +error_setg_errno(errp, -ret,
> +"device does not support requested feature x-vga");
> goto error;
> }
> }
What about the error_report() in vfio_populate_vga()? Should the
vfio_populate_vga() take an Error * argument?
> @@ -2277,7 +2277,7 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
> } else if (irq_info.count == 1) {
> vdev->pci_aer = true;
> } else {
> -error_report("vfio: %s "
> +error_report(WARN_PREFIX
> "Could not enable error recovery for the device",
> vbasedev->name);
> }
> @@ -2561,9 +2561,9 @@ static int vfio_initfn(PCIDevice *pdev)
> goto error;
> }
>
> -ret = vfio_populate_device(vdev);
> -if (ret) {
> -return ret;
> +ret = vfio_populate_device(vdev, &err);
> +if (err) {
> +goto error;
> }
>
> /* Get a copy of config space */
