* [Qemu-devel] [PATCH 01/10] savevm: use qemu_file_set_error() instead of setting last_error directly
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 02/10] QEMUFileCloseFunc: add return value documentation (v2) Eduardo Habkost
` (10 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 02/10] QEMUFileCloseFunc: add return value documentation (v2)
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 01/10] savevm: use qemu_file_set_error() instead of setting last_error directly Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 03/10] exec_close(): accept any negative value as qemu_fclose() error Eduardo Habkost
` (9 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 03/10] exec_close(): accept any negative value as qemu_fclose() error
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 01/10] savevm: use qemu_file_set_error() instead of setting last_error directly Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 02/10] QEMUFileCloseFunc: add return value documentation (v2) Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 04/10] migrate_fd_cleanup: accept any negative qemu_fclose() value as error Eduardo Habkost
` (8 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 04/10] migrate_fd_cleanup: accept any negative qemu_fclose() value as error
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (2 preceding siblings ...)
2011-11-09 22:03 ` [Qemu-devel] [PATCH 03/10] exec_close(): accept any negative value as qemu_fclose() error Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 05/10] qemu_fclose: return last_error if set (v2) Eduardo Habkost
` (7 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 05/10] qemu_fclose: return last_error if set (v2)
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (3 preceding siblings ...)
2011-11-09 22:03 ` [Qemu-devel] [PATCH 04/10] migrate_fd_cleanup: accept any negative qemu_fclose() value as error Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 06/10] stdio_pclose: return -errno on error (v2) Eduardo Habkost
` (6 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 06/10] stdio_pclose: return -errno on error (v2)
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (4 preceding siblings ...)
2011-11-09 22:03 ` [Qemu-devel] [PATCH 05/10] qemu_fclose: return last_error if set (v2) Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 07/10] stdio_fclose: return -errno on errors Eduardo Habkost
` (5 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 07/10] stdio_fclose: return -errno on errors
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (5 preceding siblings ...)
2011-11-09 22:03 ` [Qemu-devel] [PATCH 06/10] stdio_pclose: return -errno on error (v2) Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 08/10] exec_close(): " Eduardo Habkost
` (4 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 08/10] exec_close(): return -errno on errors
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (6 preceding siblings ...)
2011-11-09 22:03 ` [Qemu-devel] [PATCH 07/10] stdio_fclose: return -errno on errors Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-09 22:03 ` [Qemu-devel] [PATCH 09/10] tcp_close(): check for close() errors too Eduardo Habkost
` (3 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 09/10] tcp_close(): check for close() errors too
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (7 preceding siblings ...)
2011-11-09 22:03 ` [Qemu-devel] [PATCH 08/10] exec_close(): " Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-10 8:21 ` Dong Xu Wang
2011-11-09 22:03 ` [Qemu-devel] [PATCH 10/10] unix_close(): " Eduardo Habkost
` (2 subsequent siblings)
11 siblings, 1 reply; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 09/10] tcp_close(): check for close() errors too
2011-11-09 22:03 ` [Qemu-devel] [PATCH 09/10] tcp_close(): check for close() errors too Eduardo Habkost
@ 2011-11-10 8:21 ` Dong Xu Wang
2011-11-10 12:36 ` Eduardo Habkost
0 siblings, 1 reply; 16+ messages in thread
From: Dong Xu Wang @ 2011-11-10 8:21 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Juan Quintela, qemu-devel, Michael Roth
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 09/10] tcp_close(): check for close() errors too
2011-11-10 8:21 ` Dong Xu Wang
@ 2011-11-10 12:36 ` Eduardo Habkost
0 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-10 12:36 UTC (permalink / raw)
To: Dong Xu Wang; +Cc: Michael Roth, qemu-devel, Juan Quintela
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 10/10] unix_close(): check for close() errors too
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (8 preceding siblings ...)
2011-11-09 22:03 ` [Qemu-devel] [PATCH 09/10] tcp_close(): check for close() errors too Eduardo Habkost
@ 2011-11-09 22:03 ` Eduardo Habkost
2011-11-10 1:57 ` [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Anthony Liguori
2011-11-10 12:38 ` Eduardo Habkost
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-09 22:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2)
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (9 preceding siblings ...)
2011-11-09 22:03 ` [Qemu-devel] [PATCH 10/10] unix_close(): " Eduardo Habkost
@ 2011-11-10 1:57 ` Anthony Liguori
2011-11-10 12:04 ` Eduardo Habkost
2011-11-10 12:38 ` Eduardo Habkost
11 siblings, 1 reply; 16+ messages in thread
From: Anthony Liguori @ 2011-11-10 1:57 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Juan Quintela, qemu-devel, 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(-)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2)
2011-11-10 1:57 ` [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Anthony Liguori
@ 2011-11-10 12:04 ` Eduardo Habkost
0 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-10 12:04 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Juan Quintela, qemu-devel, Michael Roth
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2)
2011-11-09 22:03 [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Eduardo Habkost
` (10 preceding siblings ...)
2011-11-10 1:57 ` [Qemu-devel] [PATCH 00/10] qemu_fclose() error handling fixes (v2) Anthony Liguori
@ 2011-11-10 12:38 ` Eduardo Habkost
11 siblings, 0 replies; 16+ messages in thread
From: Eduardo Habkost @ 2011-11-10 12:38 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Juan Quintela
I will send v3 of this series with changes to comply with coding style,
fixing warnings detected by checkpatch.pl.
--
Eduardo
^ permalink raw reply [flat|nested] 16+ messages in thread