[PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()

Cédric Le Goater posted 26 patches 8 months, 3 weeks ago
Maintainers: Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Alex Williamson <alex.williamson@redhat.com>, Peter Xu <peterx@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Fabiano Rosas <farosas@suse.de>, Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>, Eric Blake <eblake@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, John Snow <jsnow@redhat.com>, Hyman Huang <yong.huang@smartx.com>
There is a newer version of this series
[PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()
Posted by Cédric Le Goater 8 months, 3 weeks ago
This will prepare ground for futur changes adding an Error** argument
to the save_setup() handler. We need to make sure that on failure,
vfio_save_setup() always sets a new error.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/migration.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 2050ac8897231ff89cc223f0570d5c7a65dede9e..51bea536cc290ba0aa393f78b017b0650e333bff 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -383,6 +383,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
     VFIODevice *vbasedev = opaque;
     VFIOMigration *migration = vbasedev->migration;
     uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
+    int ret;
 
     qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
 
@@ -397,13 +398,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
     }
 
     if (vfio_precopy_supported(vbasedev)) {
-        int ret;
-
         switch (migration->device_state) {
         case VFIO_DEVICE_STATE_RUNNING:
             ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
                                            VFIO_DEVICE_STATE_RUNNING);
             if (ret) {
+                error_report("%s: Failed to set new RUNNING state",
+                             vbasedev->name);
                 return ret;
             }
 
@@ -414,6 +415,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
             /* vfio_save_complete_precopy() will go to STOP_COPY */
             break;
         default:
+            error_report("%s: Invalid device state %d", vbasedev->name,
+                         migration->device_state);
             return -EINVAL;
         }
     }
@@ -422,7 +425,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
 
     qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
 
-    return qemu_file_get_error(f);
+    ret = qemu_file_get_error(f);
+    if (ret) {
+        error_report("%s: save setup failed : %s", vbasedev->name,
+                     strerror(ret));
+    }
+
+    return ret;
 }
 
 static void vfio_save_cleanup(void *opaque)
-- 
2.44.0


Re: [PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()
Posted by Avihai Horon 8 months, 3 weeks ago
On 04/03/2024 14:28, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> This will prepare ground for futur changes adding an Error** argument
> to the save_setup() handler. We need to make sure that on failure,
> vfio_save_setup() always sets a new error.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/migration.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 2050ac8897231ff89cc223f0570d5c7a65dede9e..51bea536cc290ba0aa393f78b017b0650e333bff 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -383,6 +383,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>       VFIODevice *vbasedev = opaque;
>       VFIOMigration *migration = vbasedev->migration;
>       uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
> +    int ret;
>
>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
>
> @@ -397,13 +398,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>       }
>
>       if (vfio_precopy_supported(vbasedev)) {
> -        int ret;
> -
>           switch (migration->device_state) {
>           case VFIO_DEVICE_STATE_RUNNING:
>               ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
>                                              VFIO_DEVICE_STATE_RUNNING);
>               if (ret) {
> +                error_report("%s: Failed to set new RUNNING state",
> +                             vbasedev->name);
>                   return ret;
>               }
>
> @@ -414,6 +415,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>               /* vfio_save_complete_precopy() will go to STOP_COPY */
>               break;
>           default:
> +            error_report("%s: Invalid device state %d", vbasedev->name,
> +                         migration->device_state);
>               return -EINVAL;
>           }
>       }
> @@ -422,7 +425,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>
>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>
> -    return qemu_file_get_error(f);
> +    ret = qemu_file_get_error(f);
> +    if (ret) {
> +        error_report("%s: save setup failed : %s", vbasedev->name,
> +                     strerror(ret));

Here it should be -ret (and also later in patch #12).

Thanks.

> +    }
> +
> +    return ret;
>   }
>
>   static void vfio_save_cleanup(void *opaque)
> --
> 2.44.0
>

Re: [PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()
Posted by Cédric Le Goater 8 months, 3 weeks ago
On 3/6/24 10:56, Avihai Horon wrote:
> 
> On 04/03/2024 14:28, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> This will prepare ground for futur changes adding an Error** argument
>> to the save_setup() handler. We need to make sure that on failure,
>> vfio_save_setup() always sets a new error.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/vfio/migration.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 2050ac8897231ff89cc223f0570d5c7a65dede9e..51bea536cc290ba0aa393f78b017b0650e333bff 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -383,6 +383,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>       VFIODevice *vbasedev = opaque;
>>       VFIOMigration *migration = vbasedev->migration;
>>       uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
>> +    int ret;
>>
>>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
>>
>> @@ -397,13 +398,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>       }
>>
>>       if (vfio_precopy_supported(vbasedev)) {
>> -        int ret;
>> -
>>           switch (migration->device_state) {
>>           case VFIO_DEVICE_STATE_RUNNING:
>>               ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
>>                                              VFIO_DEVICE_STATE_RUNNING);
>>               if (ret) {
>> +                error_report("%s: Failed to set new RUNNING state",
>> +                             vbasedev->name);
>>                   return ret;
>>               }
>>
>> @@ -414,6 +415,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>               /* vfio_save_complete_precopy() will go to STOP_COPY */
>>               break;
>>           default:
>> +            error_report("%s: Invalid device state %d", vbasedev->name,
>> +                         migration->device_state);
>>               return -EINVAL;
>>           }
>>       }
>> @@ -422,7 +425,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>
>>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>>
>> -    return qemu_file_get_error(f);
>> +    ret = qemu_file_get_error(f);
>> +    if (ret) {
>> +        error_report("%s: save setup failed : %s", vbasedev->name,
>> +                     strerror(ret));
> 
> Here it should be -ret (and also later in patch #12).

Yes this is like qemu_fflush(). I will also change the test to

   if (ret < 0)

As Prasad suggested.


Thanks,

C.


Re: [PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()
Posted by Avihai Horon 8 months, 3 weeks ago
On 06/03/2024 11:56, Avihai Horon wrote:
>
> On 04/03/2024 14:28, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> This will prepare ground for futur changes adding an Error** argument
>> to the save_setup() handler. We need to make sure that on failure,
>> vfio_save_setup() always sets a new error.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/vfio/migration.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 
>> 2050ac8897231ff89cc223f0570d5c7a65dede9e..51bea536cc290ba0aa393f78b017b0650e333bff 
>> 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -383,6 +383,7 @@ static int vfio_save_setup(QEMUFile *f, void 
>> *opaque)
>>       VFIODevice *vbasedev = opaque;
>>       VFIOMigration *migration = vbasedev->migration;
>>       uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
>> +    int ret;
>>
>>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
>>
>> @@ -397,13 +398,13 @@ static int vfio_save_setup(QEMUFile *f, void 
>> *opaque)
>>       }
>>
>>       if (vfio_precopy_supported(vbasedev)) {
>> -        int ret;
>> -
>>           switch (migration->device_state) {
>>           case VFIO_DEVICE_STATE_RUNNING:
>>               ret = vfio_migration_set_state(vbasedev, 
>> VFIO_DEVICE_STATE_PRE_COPY,
>> VFIO_DEVICE_STATE_RUNNING);
>>               if (ret) {
>> +                error_report("%s: Failed to set new RUNNING state",

Oh, sorry, forgot to mention in previous mail:
s/RUNNING/PRE_COPY

>> + vbasedev->name);
>>                   return ret;
>>               }
>>
>> @@ -414,6 +415,8 @@ static int vfio_save_setup(QEMUFile *f, void 
>> *opaque)
>>               /* vfio_save_complete_precopy() will go to STOP_COPY */
>>               break;
>>           default:
>> +            error_report("%s: Invalid device state %d", vbasedev->name,
>> +                         migration->device_state);
>>               return -EINVAL;
>>           }
>>       }
>> @@ -422,7 +425,13 @@ static int vfio_save_setup(QEMUFile *f, void 
>> *opaque)
>>
>>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>>
>> -    return qemu_file_get_error(f);
>> +    ret = qemu_file_get_error(f);
>> +    if (ret) {
>> +        error_report("%s: save setup failed : %s", vbasedev->name,
>> +                     strerror(ret));
>
> Here it should be -ret (and also later in patch #12).
>
> Thanks.
>
>> +    }
>> +
>> +    return ret;
>>   }
>>
>>   static void vfio_save_cleanup(void *opaque)
>> -- 
>> 2.44.0
>>

Re: [PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()
Posted by Cédric Le Goater 8 months, 3 weeks ago
On 3/6/24 11:16, Avihai Horon wrote:
> 
> On 06/03/2024 11:56, Avihai Horon wrote:
>>
>> On 04/03/2024 14:28, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> This will prepare ground for futur changes adding an Error** argument
>>> to the save_setup() handler. We need to make sure that on failure,
>>> vfio_save_setup() always sets a new error.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   hw/vfio/migration.c | 15 ++++++++++++---
>>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 2050ac8897231ff89cc223f0570d5c7a65dede9e..51bea536cc290ba0aa393f78b017b0650e333bff 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -383,6 +383,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>>       VFIODevice *vbasedev = opaque;
>>>       VFIOMigration *migration = vbasedev->migration;
>>>       uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
>>> +    int ret;
>>>
>>>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
>>>
>>> @@ -397,13 +398,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>>       }
>>>
>>>       if (vfio_precopy_supported(vbasedev)) {
>>> -        int ret;
>>> -
>>>           switch (migration->device_state) {
>>>           case VFIO_DEVICE_STATE_RUNNING:
>>>               ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_PRE_COPY,
>>> VFIO_DEVICE_STATE_RUNNING);
>>>               if (ret) {
>>> +                error_report("%s: Failed to set new RUNNING state",
> 
> Oh, sorry, forgot to mention in previous mail:
> s/RUNNING/PRE_COPY

Ah yes. Parameters are <new> and <recover> state.

Thanks,

C.




> 
>>> + vbasedev->name);
>>>                   return ret;
>>>               }
>>>
>>> @@ -414,6 +415,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>>               /* vfio_save_complete_precopy() will go to STOP_COPY */
>>>               break;
>>>           default:
>>> +            error_report("%s: Invalid device state %d", vbasedev->name,
>>> +                         migration->device_state);
>>>               return -EINVAL;
>>>           }
>>>       }
>>> @@ -422,7 +425,13 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>>
>>>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>>>
>>> -    return qemu_file_get_error(f);
>>> +    ret = qemu_file_get_error(f);
>>> +    if (ret) {
>>> +        error_report("%s: save setup failed : %s", vbasedev->name,
>>> +                     strerror(ret));
>>
>> Here it should be -ret (and also later in patch #12).
>>
>> Thanks.
>>
>>> +    }
>>> +
>>> +    return ret;
>>>   }
>>>
>>>   static void vfio_save_cleanup(void *opaque)
>>> -- 
>>> 2.44.0
>>>
> 


Re: [PATCH v3 02/26] vfio: Always report an error in vfio_save_setup()
Posted by Fabiano Rosas 8 months, 3 weeks ago
Cédric Le Goater <clg@redhat.com> writes:

> This will prepare ground for futur changes adding an Error** argument
> to the save_setup() handler. We need to make sure that on failure,
> vfio_save_setup() always sets a new error.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>