qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2)
Date: Wed, 09 Nov 2011 19:57:35 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Lightning/1.0b2 Thunderbird/3.1.13

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]