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

[Anthony Liguori <anthony@codemonkey.ws>]

On 11/10/2011 06:41 AM, Eduardo Habkost wrote:
> 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.

Applied all.  Thanks.

Regards,

Anthony Liguori

>
> --------
> 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(-)
>