Re: [PATCH 08/14] vfio: Use new Error** argument in vfio_save_setup()

[Avihai Horon]

On 12/02/2024 19:54, Cédric Le Goater wrote:
External email: Use caution opening links or attachments


On 2/12/24 10:17, Avihai Horon wrote:
Hi Cedric,

On 07/02/2024 15:33, Cédric Le Goater wrote:
External email: Use caution opening links or attachments


Add an Error** argument to vfio_migration_set_state() and adjust
callers, including vfio_save_setup(). The error will be propagated up
to qemu_savevm_state_setup() where the save_setup() handler is
executed.

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

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 
2dfbe671f6f45aa530c7341177bb532d8292cecd..2e0a79967cc97f44d9be5575c3cfe18c9f349dab 
100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -84,7 +84,8 @@ static const char *mig_state_to_str(enum 
vfio_device_mig_state state)

  static int vfio_migration_set_state(VFIODevice *vbasedev,
                                      enum vfio_device_mig_state 
new_state,
-                                    enum vfio_device_mig_state 
recover_state)
+                                    enum vfio_device_mig_state 
recover_state,
+                                    Error **errp)
  {
      VFIOMigration *migration = vbasedev->migration;
      uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
@@ -104,15 +105,15 @@ static int vfio_migration_set_state(VFIODevice 
*vbasedev,
          ret = -errno;

          if (recover_state == VFIO_DEVICE_STATE_ERROR) {
-            error_report("%s: Failed setting device state to %s, 
err: %s. "
-                         "Recover state is ERROR. Resetting device",
-                         vbasedev->name, mig_state_to_str(new_state),
-                         strerror(errno));
+            error_setg(errp, "%s: Failed setting device state to 
%s, err: %s. "
+                       "Recover state is ERROR. Resetting device",
+                       vbasedev->name, mig_state_to_str(new_state),
+                       strerror(errno));

              goto reset_device;
          }

-        error_report(
+        error_setg(errp,
              "%s: Failed setting device state to %s, err: %s. 
Setting device in recover state %s",
                       vbasedev->name, mig_state_to_str(new_state),
                       strerror(errno), 
mig_state_to_str(recover_state));
@@ -120,7 +121,7 @@ static int vfio_migration_set_state(VFIODevice 
*vbasedev,
          mig_state->device_state = recover_state;
          if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
              ret = -errno;
-            error_report(
+            error_setg(errp,
                  "%s: Failed setting device in recover state, err: 
%s. Resetting device",
                           vbasedev->name, strerror(errno));

I think here we will assert because errp is already set.

Adding an error_append() API would be useful here I guess.

yes.

Otherwise, we need to move the first error_setg() below, to before we 
return from a successful recover state change, and construct the 
error message differently (e.g., provide a full error message for the 
recover state fail case containing also the first error).

Do you have other ideas?

Errors for :

    if (ioctl(vbasedev->fd, VFIO_DEVICE_RESET)) {

should be treated as the others with and error_append() and not
hw_error(). This needs a rework before any new changes.

I also wonder why we have twice :

    migration->device_state = recover_state;

Not sure where you see we have it twice.

One time we set "mig_state->device_state = recover_state;" for the ioctl.
And another time we set "migration->device_state = recover_state;" in 
case of successful ioctl, to update the device state in migration struct.


It looks redundant. The ioctl VFIO_DEVICE_FEATURE should leave the
state unmodified.

Thanks,

C.