On 11/09/2011 04:03 PM, Eduardo Habkost wrote:
I am not sure if this is appropriate post-freeze, I will let the maintainers
decide this. Personally I think the code is more reliable with these changes,
but on the other hand the only bugs it fixes are on the error paths.
What bug does this fix?
Regards,
Anthony Liguori
Changes v1 -> v2:
- Patch 2: Cosmetic spelling change on comment text
- Patch 5: Add small comment about the need to return previously-spotted
errors
- Patch 6: On success, keep returning pclose() return value, instead of
always 0.
(the most relevant change in this new version of the series)
Also, this series was tested using ping-pong migration with Autotest, no
problems were detected.
Original series description follows:
----------
Summary of the problem:
- qemu_fclose() calls qemu_fflush()
- Writes done by qemu_fflush() can fail
- Those errors are lost after qemu_fclose() returns
So, this series change qemu_fclose() to return last_error. But to do that we
need to make sure all involve code use the -errno convention, hence the large
series.
Michael, probably this will conflict with your ongoing work. I don't want to
delay other work, so I can rebase my patches if needed. This is just a RFC.
Juan, maybe you were already working on this. But as I was already fixing this
code while auditing the migration handling, I thought it was interesting to
send this for review anyway. I hope I didn't duplicate any work.
This is still completely untested, I am just using this series as a way to
report the issue and get comments so I know I am going through the right path.
Detailed description of the changes:
Small cleanups:
- Always use qemu_file_set_error() to set last_error (patch 1)
- Add return value documentation to QEMUFileCloseFunc (patch 2)
Actual qemu_fclose() behavior changes are done in 3 steps:
- First step: fix qemu_fclose() callers:
- exec_close()
- Fixed to check for negative values, not -1 (patch 3)
- Note: exec_close() is changed in two steps: first on the qemu_fclose()
calling code, then on the return value code
- migrate_fd_cleanup
- Fixed to:
- check qemu_fclose() return value for<0 (patch 4)
- return -errno, not just -1 (patch 4)
- Callers:
- migrate_fd_completed:
- Error checking is done properly, already.
- migrate_fd_error:
- It ignores migrated_fd_cleanup() return value.
- migrate_fd_cancel:
- It ignores migrated_fd_cleanup() return value.
- exec_accept_incoming_migration(): no return value check (yet)
- fd_accept_incoming_migration(): no return value check (yet)
- tcp_accept_incoming_migration(): no return value check (yet)
- unix_accept_incoming_migration(): no return value check (yet)
- do_savevm(): no return value check (yet)
- load_vmstate(): no return value check (yet)
- Second step: change qemu_fclose() to return last_error (patch 5)
- Made sure to return unchanged (positive) success value on success
(required by exec_close())
- Third step: change qemu_fclose() implementations (QEMUFileCloseFunc):
- stdio_fclose
- Fixed to return -errno (patch 6)
- stdio_pclose
- Fixed to return -errno (patch 7)
- buffered_close
- Implemented through QEMUFileBuffered.close:
- Only implementation is migrate_fd_close(), that calls the following,
through MigrationState.close:
- exec_close():
- fixed to return original error value, not -1 (patch 8)
- fd_close
- Fixed to return -errno on close() errors. (patch 9)
- tcp_close
- Fixed to return -errno on close() errors. (patch 10)
- unix_close
- Fixed to return -errno on close() errors. (patch 11)
- socket_close
- No system call is made, returns always 0.
- bdrv_fclose
- No system call is made, returns always 0.
Eduardo Habkost (10):
savevm: use qemu_file_set_error() instead of setting last_error
directly
QEMUFileCloseFunc: add return value documentation (v2)
exec_close(): accept any negative value as qemu_fclose() error
migrate_fd_cleanup: accept any negative qemu_fclose() value as error
qemu_fclose: return last_error if set (v2)
stdio_pclose: return -errno on error (v2)
stdio_fclose: return -errno on errors
exec_close(): return -errno on errors
tcp_close(): check for close() errors too
unix_close(): check for close() errors too
hw/hw.h | 8 ++++++-
migration-exec.c | 9 ++-----
migration-tcp.c | 6 +++-
migration-unix.c | 6 +++-
migration.c | 4 +--
savevm.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++------
6 files changed, 73 insertions(+), 21 deletions(-)
