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

[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

[Qemu-devel] [PATCH 01/10] savevm: use qemu_file_set_error() instead of setting last_error directly

[Eduardo Habkost]

Some code uses qemu_file_set_error() already, so use it everywhere
when setting last_error, for consistency.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 savevm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/savevm.c b/savevm.c
index bee16c0..2dab5dc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -448,7 +448,7 @@ void qemu_fflush(QEMUFile *f)
         if (len > 0)
             f->buf_offset += f->buf_index;
         else
-            f->last_error = -EINVAL;
+            qemu_file_set_error(f, -EINVAL);
         f->buf_index = 0;
     }
 }
@@ -479,7 +479,7 @@ static void qemu_fill_buffer(QEMUFile *f)
     } else if (len == 0) {
         f->last_error = -EIO;
     } else if (len != -EAGAIN)
-        f->last_error = len;
+        qemu_file_set_error(f, len);
 }
 
 int qemu_fclose(QEMUFile *f)
-- 
1.7.3.2

[Qemu-devel] [PATCH 02/10] QEMUFileCloseFunc: add return value documentation (v2)

[Eduardo Habkost]

qemu_fclose() and QEMUFile->close will return -errno on error, and any
positive value on success.

We need the positive non-zero success values because
migration-exec.c:exec_close() relies on non-zero return values to get
the process exit code.

Changes v1 -> v2:
 - Cosmetic spelling change on comment text

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/hw.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index ed20f5a..efa04d1 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -27,7 +27,13 @@ typedef int (QEMUFilePutBufferFunc)(void *opaque, const uint8_t *buf,
 typedef int (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
                                     int64_t pos, int size);
 
-/* Close a file and return an error code */
+/* Close a file
+ *
+ * Return negative error number on error, 0 or positive value on success.
+ *
+ * The meaning of return value on success depends on the specific back-end being
+ * used.
+ */
 typedef int (QEMUFileCloseFunc)(void *opaque);
 
 /* Called to determine if the file has exceeded it's bandwidth allocation.  The
-- 
1.7.3.2

[Qemu-devel] [PATCH 03/10] exec_close(): accept any negative value as qemu_fclose() error

[Eduardo Habkost]

Note that we don't return the unchanged return value back yet, because
we need to change all qemu_fclose() callers to accept any positive value
as success.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 migration-exec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index b7b1055..626b648 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -50,7 +50,7 @@ static int exec_close(MigrationState *s)
         ret = qemu_fclose(s->opaque);
         s->opaque = NULL;
         s->fd = -1;
-        if (ret != -1 &&
+        if (ret >= 0 &&
             WIFEXITED(ret)
             && WEXITSTATUS(ret) == 0) {
             ret = 0;
-- 
1.7.3.2

[Qemu-devel] [PATCH 04/10] migrate_fd_cleanup: accept any negative qemu_fclose() value as error

[Eduardo Habkost]

Also, we now return the qemu_fclose() value unchanged to the caller. For
reference, the migrate_fd_cleanup() callers are the following:

- migrate_fd_completed(): any negative value is considered an
  error, so the change is OK.
- migrate_fd_error(): doesn't check the migrate_fd_cleanup() return value
- migrate_fd_cancel(): doesn't check the migrate_fd_cleanup() return
  value

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 migration.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/migration.c b/migration.c
index 4b17566..5a33003 100644
--- a/migration.c
+++ b/migration.c
@@ -172,9 +172,7 @@ static int migrate_fd_cleanup(MigrationState *s)
 
     if (s->file) {
         DPRINTF("closing file\n");
-        if (qemu_fclose(s->file) != 0) {
-            ret = -1;
-        }
+        ret = qemu_fclose(s->file);
         s->file = NULL;
     } else {
         if (s->mon) {
-- 
1.7.3.2

[Qemu-devel] [PATCH 05/10] qemu_fclose: return last_error if set (v3)

[Eduardo Habkost]

This will make sure no error will be missed as long as callers always
check for qemu_fclose() return value. For reference, this is the
complete list of qemu_fclose() callers:

 - exec_close(): already fixed to check for negative values, not -1
 - migrate_fd_cleanup(): already fixed to consider only negative values
   as error, not any non-zero 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)

Changes v1 -> v2:
 - Add small comment about the need to return previously-spotted errors

Changes v2 -> v3:
 - Add braces to "if" statements to match coding style

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 savevm.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/savevm.c b/savevm.c
index 2dab5dc..2fef693 100644
--- a/savevm.c
+++ b/savevm.c
@@ -436,6 +436,22 @@ void qemu_file_set_error(QEMUFile *f, int ret)
     f->last_error = ret;
 }
 
+/** Sets last_error conditionally
+ *
+ * Sets last_error only if ret is negative _and_ no error
+ * was set before.
+ */
+static void qemu_file_set_if_error(QEMUFile *f, int ret)
+{
+    if (ret < 0 && !f->last_error) {
+        qemu_file_set_error(f, ret);
+    }
+}
+
+/** Flushes QEMUFile buffer
+ *
+ * In case of error, last_error is set.
+ */
 void qemu_fflush(QEMUFile *f)
 {
     if (!f->put_buffer)
@@ -482,12 +498,41 @@ static void qemu_fill_buffer(QEMUFile *f)
         qemu_file_set_error(f, len);
 }
 
-int qemu_fclose(QEMUFile *f)
+/** Calls close function and set last_error if needed
+ *
+ * Internal function. qemu_fflush() must be called before this.
+ *
+ * Returns f->close() return value, or 0 if close function is not set.
+ */
+static int qemu_close(QEMUFile *f)
 {
     int ret = 0;
-    qemu_fflush(f);
-    if (f->close)
+    if (f->close) {
         ret = f->close(f->opaque);
+        qemu_file_set_if_error(f, ret);
+    }
+    return ret;
+}
+
+/** Closes the file
+ *
+ * Returns negative error value if any error happened on previous operations or
+ * while closing the file. Returns 0 or positive number on success.
+ *
+ * The meaning of return value on success depends on the specific backend
+ * being used.
+ */
+int qemu_fclose(QEMUFile *f)
+{
+    int ret;
+    qemu_fflush(f);
+    ret = qemu_close(f);
+    /* If any error was spotted before closing, we should report it
+     * instead of the close() return value.
+     */
+    if (f->last_error) {
+        ret = f->last_error;
+    }
     g_free(f);
     return ret;
 }
-- 
1.7.3.2

[Qemu-devel] [PATCH 06/10] stdio_pclose: return -errno on error (v3)

[Eduardo Habkost]

This is what qemu_fclose() expects.

Changes v1 -> v2:
 - On success, keep returning pclose() return value, instead of always 0.

Changes v2 -> v3:
 - Add braces on if statements to match coding style

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 savevm.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/savevm.c b/savevm.c
index 2fef693..a870b3f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -235,6 +235,9 @@ static int stdio_pclose(void *opaque)
     QEMUFileStdio *s = opaque;
     int ret;
     ret = pclose(s->stdio_file);
+    if (ret == -1) {
+        ret = -errno;
+    }
     g_free(s);
     return ret;
 }
-- 
1.7.3.2

[Qemu-devel] [PATCH 07/10] stdio_fclose: return -errno on errors (v2)

[Eduardo Habkost]

This is what qemu_fclose() expects.

Changes v1 -> v2:
 - Add braces to if statement to match coding style

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 savevm.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/savevm.c b/savevm.c
index a870b3f..4ccbc1c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -245,9 +245,12 @@ static int stdio_pclose(void *opaque)
 static int stdio_fclose(void *opaque)
 {
     QEMUFileStdio *s = opaque;
-    fclose(s->stdio_file);
+    int ret = 0;
+    if (fclose(s->stdio_file) == EOF) {
+        ret = -errno;
+    }
     g_free(s);
-    return 0;
+    return ret;
 }
 
 QEMUFile *qemu_popen(FILE *stdio_file, const char *mode)
-- 
1.7.3.2

[Qemu-devel] [PATCH 08/10] exec_close(): return -errno on errors (v2)

[Eduardo Habkost]

All qemu_fclose() callers were already changed to accept any negative
value as error, so we now can change it to return -errno.

When the process exits with a non-zero exit code, we return -EIO to as a
fake errno value.

Changes v1 -> v2:
 - Don't use "//" comments, to make checkpatch.pl happy

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 migration-exec.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index 626b648..e14552e 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -50,12 +50,9 @@ static int exec_close(MigrationState *s)
         ret = qemu_fclose(s->opaque);
         s->opaque = NULL;
         s->fd = -1;
-        if (ret >= 0 &&
-            WIFEXITED(ret)
-            && WEXITSTATUS(ret) == 0) {
-            ret = 0;
-        } else {
-            ret = -1;
+        if (ret >= 0 && !(WIFEXITED(ret) && WEXITSTATUS(ret) == 0)) {
+            /* close succeeded, but non-zero exit code: */
+            ret = -EIO; /* fake errno value */
         }
     }
     return ret;
-- 
1.7.3.2

[Qemu-devel] [PATCH 09/10] tcp_close(): check for close() errors too (v2)

[Eduardo Habkost]

In case close() fails, we want to report the error back.

Changes v1 -> v2:
 - Use braces on if statement to match coding style

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 migration-tcp.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 5aa742c..cf6a9b8 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -40,12 +40,15 @@ static int socket_write(MigrationState *s, const void * buf, size_t size)
 
 static int tcp_close(MigrationState *s)
 {
+    int r = 0;
     DPRINTF("tcp_close\n");
     if (s->fd != -1) {
-        close(s->fd);
+        if (close(s->fd) < 0) {
+            r = -errno;
+        }
         s->fd = -1;
     }
-    return 0;
+    return r;
 }
 
 static void tcp_wait_for_connect(void *opaque)
-- 
1.7.3.2

[Qemu-devel] [PATCH 10/10] unix_close(): check for close() errors too (v2)

[Eduardo Habkost]

In case close() fails, we want to report the error back.

Changes v1 -> v2:
 - Use braces on if statement to match coding style

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 migration-unix.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/migration-unix.c b/migration-unix.c
index 8596353..dfcf203 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -40,12 +40,15 @@ static int unix_write(MigrationState *s, const void * buf, size_t size)
 
 static int unix_close(MigrationState *s)
 {
+    int r = 0;
     DPRINTF("unix_close\n");
     if (s->fd != -1) {
-        close(s->fd);
+        if (close(s->fd) < 0) {
+            r = -errno;
+        }
         s->fd = -1;
     }
-    return 0;
+    return r;
 }
 
 static void unix_wait_for_connect(void *opaque)
-- 
1.7.3.2

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

[Anthony Liguori]

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