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;
It looks redundant. The ioctl VFIO_DEVICE_FEATURE should leave the
state unmodified.
Thanks,
C.
