From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RaAcU-000822-Le for qemu-devel@nongnu.org; Mon, 12 Dec 2011 13:28:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RaAcT-00066g-Ap for qemu-devel@nongnu.org; Mon, 12 Dec 2011 13:28:58 -0500 Received: from mail-iy0-f173.google.com ([209.85.210.173]:46846) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RaAcT-00066X-35 for qemu-devel@nongnu.org; Mon, 12 Dec 2011 13:28:57 -0500 Received: by iagj37 with SMTP id j37so3049318iag.4 for ; Mon, 12 Dec 2011 10:28:56 -0800 (PST) Message-ID: <4EE647E5.5000605@codemonkey.ws> Date: Mon, 12 Dec 2011 12:28:53 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1320928908-19076-1-git-send-email-ehabkost@redhat.com> In-Reply-To: <1320928908-19076-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 (v3) 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/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(-) >