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

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

[Eduardo Habkost]

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

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

[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

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

diff --git a/savevm.c b/savevm.c
index 2dab5dc..6e60c18 100644
--- a/savevm.c
+++ b/savevm.c
@@ -436,6 +436,21 @@ 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 +497,40 @@ 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 (v2)

[Eduardo Habkost]

This is what qemu_fclose() expects.

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

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

diff --git a/savevm.c b/savevm.c
index 6e60c18..5112d66 100644
--- a/savevm.c
+++ b/savevm.c
@@ -235,6 +235,8 @@ 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

[Eduardo Habkost]

This is what qemu_fclose() expects.

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

diff --git a/savevm.c b/savevm.c
index 5112d66..ee49288 100644
--- a/savevm.c
+++ b/savevm.c
@@ -244,9 +244,11 @@ 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

[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.

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..1e9cb1d 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

[Eduardo Habkost]

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

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

diff --git a/migration-tcp.c b/migration-tcp.c
index 5aa742c..fd5fd56 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -40,12 +40,14 @@ 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

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

[Dong Xu Wang]

2011/11/10 Eduardo Habkost <ehabkost@redhat.com>:
> In case close() fails, we want to report the error back.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  migration-tcp.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 5aa742c..fd5fd56 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @ -40,12 +40,14 @@ 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;
Braces {} are necessary, or checkpatch.pl will give warnings, also in
other patches in your series.

No mistake, just reminder. :)
>         s->fd = -1;
>     }
> -    return 0;
> +    return r;
>  }
>
>  static void tcp_wait_for_connect(void *opaque)
> --
> 1.7.3.2
>
>
>



-- 
Regards
Robert Wang

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

[Eduardo Habkost]

On Thu, Nov 10, 2011 at 04:21:46PM +0800, Dong Xu Wang wrote:
> >     if (s->fd != -1) {
> > -        close(s->fd);
> > +        if (close(s->fd) < 0)
> > +            r = -errno;
> Braces {} are necessary, or checkpatch.pl will give warnings, also in
> other patches in your series.
> 
> No mistake, just reminder. :)

I will send v3 with the coding style fixed (I was supposed to fix this
before submitting v2 but I forgot). Thanks!

-- 
Eduardo

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

[Eduardo Habkost]

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

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

diff --git a/migration-unix.c b/migration-unix.c
index 8596353..197285c 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -40,12 +40,14 @@ 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 (v2)

[Anthony Liguori]

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

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

[Eduardo Habkost]

On Wed, Nov 09, 2011 at 07:57:35PM -0600, Anthony Liguori wrote:
> 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?

This one:

> >- qemu_fclose() calls qemu_fflush()
> >- Writes done by qemu_fflush() can fail
> >- Those errors are lost after qemu_fclose() returns

(The fix for that is on patch 05/10, the rest are related cleanups or
dependencies for the actual fix).

Also, errno values were being lost in the call chain as most functions
were returning just -1 instead of -errno.

I don't have any specific bug report that this fixes, but we already had
multiple reports on RHEL of errors during migration (e.g. interrupted
network connection) causing hangs. This is only part of the changes
required to make migration error handling reliable.

-- 
Eduardo

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

[Eduardo Habkost]

I will send v3 of this series with changes to comply with coding style,
fixing warnings detected by checkpatch.pl.

-- 
Eduardo

[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 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