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

[Eduardo Habkost]

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