Re: [Qemu-devel] [RFC PATCH 00/11] qemu_fclose() error handling fixes

[Michael Roth]

On Tue,  1 Nov 2011 17:20:19 -0200, Eduardo Habkost <address@hidden> wrote:
> 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.

No worries, my stuff is probably a ways off whereas this is likely a
candidate for 1.0 or a follow-up stable revision. Plus I've gotten pretty used
to working through QEMUFile-related merge conflicts as of late :)

Still reviewing, but series looks good so far (other than 6)

> 
> 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 (11):
>   savevm: use qemu_file_set_error() instead of setting last_error
>     directly
>   QEMUFileCloseFunc: add return value documentation
>   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
>   stdio_pclose: return -errno on error
>   stdio_fclose: return -errno on errors
>   exec_close(): return -errno on errors
>   fd_close(): check for close() errors too
>   tcp_close(): check for close() errors too
>   unix_close(): check for close() errors too
> 
>  hw/hw.h          |    8 ++++++-
>  migration-exec.c |    9 ++-----
>  migration-fd.c   |    6 +++-
>  migration-tcp.c  |    6 +++-
>  migration-unix.c |    6 +++-
>  migration.c      |    4 +--
>  savevm.c         |   61 
> ++++++++++++++++++++++++++++++++++++++++++++++--------
>  7 files changed, 75 insertions(+), 25 deletions(-)
> 
> -- 
> 1.7.3.2
> 

-- 
Sincerely,
Mike Roth
IBM Linux Technology Center