[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v3)
From: |
Eduardo Habkost |
Subject: |
[Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v3) |
Date: |
Thu, 10 Nov 2011 10:41:38 -0200 |
Comments for v3:
I am still not sure if this is 1.0 material, but I am more inclined to delay
this for post-1.0.
Changes v2 -> v3:
- Only coding style changes for issues detected by checkpatch.pl:
- Avoid "//" comments;
- Use braces on if statements.
--------
Comments for v2:
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.
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:
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 (v3)
stdio_pclose: return -errno on error (v3)
stdio_fclose: return -errno on errors (v2)
exec_close(): return -errno on errors (v2)
tcp_close(): check for close() errors too (v2)
unix_close(): check for close() errors too (v2)
hw/hw.h | 8 +++++-
migration-exec.c | 9 ++-----
migration-tcp.c | 7 ++++-
migration-unix.c | 7 ++++-
migration.c | 4 +--
savevm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++-----
6 files changed, 79 insertions(+), 21 deletions(-)
--
1.7.3.2
- [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v3),
Eduardo Habkost <=
- [Qemu-devel] [PATCH 02/10] QEMUFileCloseFunc: add return value documentation (v2), Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 04/10] migrate_fd_cleanup: accept any negative qemu_fclose() value as error, Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 01/10] savevm: use qemu_file_set_error() instead of setting last_error directly, Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 09/10] tcp_close(): check for close() errors too (v2), Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 06/10] stdio_pclose: return -errno on error (v3), Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 05/10] qemu_fclose: return last_error if set (v3), Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 03/10] exec_close(): accept any negative value as qemu_fclose() error, Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 07/10] stdio_fclose: return -errno on errors (v2), Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 10/10] unix_close(): check for close() errors too (v2), Eduardo Habkost, 2011/11/10
- [Qemu-devel] [PATCH 08/10] exec_close(): return -errno on errors (v2), Eduardo Habkost, 2011/11/10