[Qemu-devel] [RFC PATCH 00/11] qemu_fclose() error handling fixes

[Qemu-devel] [RFC PATCH 00/11] qemu_fclose() error handling fixes

[Eduardo Habkost]

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 (11):
  savevm: use qemu_file_set_error() instead of setting last_error
    directly
  QEMUFileCloseFunc: add return value documentation
  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
  stdio_pclose: return -errno on error
  stdio_fclose: return -errno on errors
  exec_close(): return -errno on errors
  fd_close(): check for close() errors too
  tcp_close(): check for close() errors too
  unix_close(): check for close() errors too

 hw/hw.h          |    8 ++++++-
 migration-exec.c |    9 ++-----
 migration-fd.c   |    6 +++-
 migration-tcp.c  |    6 +++-
 migration-unix.c |    6 +++-
 migration.c      |    4 +--
 savevm.c         |   61 ++++++++++++++++++++++++++++++++++++++++++++++--------
 7 files changed, 75 insertions(+), 25 deletions(-)

-- 
1.7.3.2

[Qemu-devel] [RFC PATCH 01/11] 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 f01838f..dc3311b 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;
     }
 }
@@ -477,7 +477,7 @@ static void qemu_fill_buffer(QEMUFile *f)
         f->buf_size += len;
         f->buf_offset += len;
     } 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] [RFC PATCH 02/11] QEMUFileCloseFunc: add return value documentation

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

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..2e36223 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 backend 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] [RFC PATCH 03/11] 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] [RFC PATCH 04/11] 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 3b4abbd..a98897c 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] [RFC PATCH 05/11] qemu_fclose: return last_error if set

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

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

diff --git a/savevm.c b/savevm.c
index dc3311b..3c746a6 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)
@@ -480,12 +495,37 @@ 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 (f->last_error)
+        ret = f->last_error;
     g_free(f);
     return ret;
 }
-- 
1.7.3.2

[Qemu-devel] [RFC PATCH 06/11] stdio_pclose: return -errno on error

[Eduardo Habkost]

This is what qemu_fclose() expects.

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

diff --git a/savevm.c b/savevm.c
index 3c746a6..63dd719 100644
--- a/savevm.c
+++ b/savevm.c
@@ -233,8 +233,9 @@ static int stdio_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
 static int stdio_pclose(void *opaque)
 {
     QEMUFileStdio *s = opaque;
-    int ret;
-    ret = pclose(s->stdio_file);
+    int ret = 0;
+    if (pclose(s->stdio_file) == -1)
+        ret = -errno;
     g_free(s);
     return ret;
 }
-- 
1.7.3.2

[Qemu-devel] [RFC PATCH 07/11] 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 63dd719..7fbc4ca 100644
--- a/savevm.c
+++ b/savevm.c
@@ -243,9 +243,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] [RFC PATCH 08/11] 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] [RFC PATCH 09/11] fd_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-fd.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration-fd.c b/migration-fd.c
index d0aec89..4d86d43 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -42,12 +42,14 @@ static int fd_write(MigrationState *s, const void * buf, size_t size)
 
 static int fd_close(MigrationState *s)
 {
+    int r = 0;
     DPRINTF("fd_close\n");
     if (s->fd != -1) {
-        close(s->fd);
+        if (close(s->fd) < 0)
+            r = -errno;
         s->fd = -1;
     }
-    return 0;
+    return r;
 }
 
 int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
-- 
1.7.3.2

[Qemu-devel] [RFC PATCH 10/11] 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

[Qemu-devel] [RFC PATCH 11/11] 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] [RFC PATCH 05/11] qemu_fclose: return last_error if set

[Paolo Bonzini]

On 11/01/2011 08:20 PM, Eduardo Habkost wrote:
> +/** 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);

Isn't the return value of qemu_close useless, because if nonzero it will 
always be present in last_error too?  With this change, I'd leave it inline.

> +    if (f->last_error)
> +        ret = f->last_error;
>       g_free(f);
>       return ret;

Paolo

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

[Juan Quintela]

Eduardo Habkost <ehabkost@redhat.com> wrote:
> 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 (11):
>   savevm: use qemu_file_set_error() instead of setting last_error
>     directly
>   QEMUFileCloseFunc: add return value documentation
>   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
>   stdio_pclose: return -errno on error
>   stdio_fclose: return -errno on errors
>   exec_close(): return -errno on errors
>   fd_close(): check for close() errors too
>   tcp_close(): check for close() errors too
>   unix_close(): check for close() errors too
>
>  hw/hw.h          |    8 ++++++-
>  migration-exec.c |    9 ++-----
>  migration-fd.c   |    6 +++-
>  migration-tcp.c  |    6 +++-
>  migration-unix.c |    6 +++-
>  migration.c      |    4 +--
>  savevm.c         |   61 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  7 files changed, 75 insertions(+), 25 deletions(-)

ACK series

Re: [Qemu-devel] [RFC PATCH 05/11] qemu_fclose: return last_error if set

[Eduardo Habkost]

On Wed, Nov 02, 2011 at 08:33:05AM +0100, Paolo Bonzini wrote:
> On 11/01/2011 08:20 PM, Eduardo Habkost wrote:
> >+/** 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);
> 
> Isn't the return value of qemu_close useless, because if nonzero it
> will always be present in last_error too?  With this change, I'd
> leave it inline.

No, if it's positive it won't be set on last_error, so we have to save
it somewhere other than last_error (that's what the qemu_close() return
value is used for).


It could be inlined, yes. I just wanted to isolate the "call f->close if
set, handling errors" part from the flush+close+g_free logic, to try to
make functions shorter and easier to read and analyze (please let me
know if I failed to do that :-).


Without a separate function and qemu_file_set_if_error(), the function
will look like:

int qemu_fclose(QEMUFile *f)
{
    int ret = 0;
    qemu_fflush();
    if (f->close) {
        ret = f->close(f->opaque);
    }
    if (f->last_error) {
        ret = f->last_error;
    }
    g_free(f);
    return ret;
}


Now that I am looking at the resulting code, it doesn't look too bad. I
guess I was simply too eager to encapsulate every bit of logic (in this
case the "if (f->close) ..." part) into separate functions. I find the
two-function version slightly easier to analyze, though.

I am happy with either version, so I am open to suggestions.


> 
> >+    if (f->last_error)
> >+        ret = f->last_error;

Oops, I have to fix coding style and add braces here.

> >      g_free(f);
> >      return ret;
> 
> Paolo
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 05/11] qemu_fclose: return last_error if set

[Paolo Bonzini]

On 11/02/2011 12:56 PM, Eduardo Habkost wrote:
> No, if it's positive it won't be set on last_error, so we have to save
> it somewhere other than last_error (that's what the qemu_close() return
> value is used for).

Ok, I was confused by your patch 6, which basically removes the only 
case when qemu_fclose was returning a positive, nonzero value. :)  I 
guess the problem is there?

> Without a separate function and qemu_file_set_if_error(), the function
> will look like:
>
> int qemu_fclose(QEMUFile *f)
> {
>      int ret = 0;
>      qemu_fflush();
>      if (f->close) {
>          ret = f->close(f->opaque);
>      }
>      if (f->last_error) {
            ^^^^^^^^^^^^^

"if (ret >= 0 && f->last_error)" perhaps?

>          ret = f->last_error;
>      }
>      g_free(f);
>      return ret;
> }
>
>
> Now that I am looking at the resulting code, it doesn't look too bad. I
> guess I was simply too eager to encapsulate every bit of logic (in this
> case the "if (f->close) ..." part) into separate functions. I find the
> two-function version slightly easier to analyze, though.

Yes, it's the same for me too now that I actually understand what's 
going on.

Paolo

Re: [Qemu-devel] [RFC PATCH 05/11] qemu_fclose: return last_error if set

[Eduardo Habkost]

On Wed, Nov 02, 2011 at 01:15:46PM +0100, Paolo Bonzini wrote:
> On 11/02/2011 12:56 PM, Eduardo Habkost wrote:
> >No, if it's positive it won't be set on last_error, so we have to save
> >it somewhere other than last_error (that's what the qemu_close() return
> >value is used for).
> 
> Ok, I was confused by your patch 6, which basically removes the only
> case when qemu_fclose was returning a positive, nonzero value. :)  I
> guess the problem is there?

Yes! We must return the pclose() return value there, as exec_close()
needs it. Thanks for spotting it.  :-)

> 
> >Without a separate function and qemu_file_set_if_error(), the function
> >will look like:
> >
> >int qemu_fclose(QEMUFile *f)
> >{
> >     int ret = 0;
> >     qemu_fflush();
> >     if (f->close) {
> >         ret = f->close(f->opaque);
> >     }
> >     if (f->last_error) {
>            ^^^^^^^^^^^^^
> 
> "if (ret >= 0 && f->last_error)" perhaps?

I don't think so: if f->close() fails but we have already got an error
on any previous operation (in other words, if f->last_error was already
set), I think we should return info about the first error (that will
probably be more informative), instead of the close() error.

> 
> >         ret = f->last_error;
> >     }
> >     g_free(f);
> >     return ret;
> >}
> >
> >
> >Now that I am looking at the resulting code, it doesn't look too bad. I
> >guess I was simply too eager to encapsulate every bit of logic (in this
> >case the "if (f->close) ..." part) into separate functions. I find the
> >two-function version slightly easier to analyze, though.
> 
> Yes, it's the same for me too now that I actually understand what's
> going on.

Good.  :-)

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 06/11] stdio_pclose: return -errno on error

[Eduardo Habkost]

On Tue, Nov 01, 2011 at 05:20:25PM -0200, Eduardo Habkost wrote:
> This is what qemu_fclose() expects.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  savevm.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 3c746a6..63dd719 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -233,8 +233,9 @@ static int stdio_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
>  static int stdio_pclose(void *opaque)
>  {
>      QEMUFileStdio *s = opaque;
> -    int ret;
> -    ret = pclose(s->stdio_file);
> +    int ret = 0;
> +    if (pclose(s->stdio_file) == -1)
> +        ret = -errno;
>      g_free(s);
>      return ret;

Self-NACK. We have to return the pclose() return value here, because
exec_close() needs it.

Thanks to Paolo for spotting it.

-- 
Eduardo

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

[Michael Roth]

On Tue,  1 Nov 2011 17:20:19 -0200, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 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.

No worries, my stuff is probably a ways off whereas this is likely a
candidate for 1.0 or a follow-up stable revision. Plus I've gotten pretty used
to working through QEMUFile-related merge conflicts as of late :)

Still reviewing, but series looks good so far (other than 6)

> 
> 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 (11):
>   savevm: use qemu_file_set_error() instead of setting last_error
>     directly
>   QEMUFileCloseFunc: add return value documentation
>   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
>   stdio_pclose: return -errno on error
>   stdio_fclose: return -errno on errors
>   exec_close(): return -errno on errors
>   fd_close(): check for close() errors too
>   tcp_close(): check for close() errors too
>   unix_close(): check for close() errors too
> 
>  hw/hw.h          |    8 ++++++-
>  migration-exec.c |    9 ++-----
>  migration-fd.c   |    6 +++-
>  migration-tcp.c  |    6 +++-
>  migration-unix.c |    6 +++-
>  migration.c      |    4 +--
>  savevm.c         |   61 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  7 files changed, 75 insertions(+), 25 deletions(-)
> 
> -- 
> 1.7.3.2
> 

-- 
Sincerely,
Mike Roth
IBM Linux Technology Center