From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49224) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ROJtc-0001sR-Vy for qemu-devel@nongnu.org; Wed, 09 Nov 2011 20:57:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ROJtb-0003AS-81 for qemu-devel@nongnu.org; Wed, 09 Nov 2011 20:57:40 -0500 Received: from mail-yx0-f173.google.com ([209.85.213.173]:58266) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ROJtb-00039u-4Y for qemu-devel@nongnu.org; Wed, 09 Nov 2011 20:57:39 -0500 Received: by yenr8 with SMTP id r8so1549181yen.4 for ; Wed, 09 Nov 2011 17:57:38 -0800 (PST) Message-ID: <4EBB2F8F.7070807@redhat.com> Date: Wed, 09 Nov 2011 19:57:35 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1320876205-16113-1-git-send-email-ehabkost@redhat.com> In-Reply-To: <1320876205-16113-1-git-send-email-ehabkost@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Juan Quintela , qemu-devel@nongnu.org, Michael Roth 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(-) >