migration/migration.c | 10 ++++-- migration/qemu-file-channel.c | 30 +++++++++-------- migration/qemu-file.c | 63 ++++++++++++++++++++++++++++------- migration/qemu-file.h | 15 ++++++--- migration/savevm.c | 6 ++-- 5 files changed, 88 insertions(+), 36 deletions(-)
Currently, there is no information about error if outgoing migration was failed
because of file channel errors.
Example (QMP session):
-> { "execute": "migrate", "arguments": { "uri": "exec:head -c 1" }}
<- { "return": {} }
...
-> { "execute": "query-migrate" }
<- { "return": { "status": "failed" }} // There is not error's description
And even in the QEMU's output there is nothing.
This patch
1) Adds errp for the most of QEMUFileOps
2) Adds qemu_file_get_error_obj/qemu_file_set_error_obj
3) And finally using of qemu_file_get_error_obj in migration.c
And now, the status for the mentioned fail will be:
-> { "execute": "query-migrate" }
<- { "return": { "status": "failed",
"error-desc": "Unable to write to command: Broken pipe" }}
Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
migration/migration.c | 10 ++++--
migration/qemu-file-channel.c | 30 +++++++++--------
migration/qemu-file.c | 63 ++++++++++++++++++++++++++++-------
migration/qemu-file.h | 15 ++++++---
migration/savevm.c | 6 ++--
5 files changed, 88 insertions(+), 36 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 609e0df5d0..7bcdc4613b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2949,6 +2949,7 @@ static MigThrError migration_detect_error(MigrationState *s)
{
int ret;
int state = s->state;
+ Error *local_error = NULL;
if (state == MIGRATION_STATUS_CANCELLING ||
state == MIGRATION_STATUS_CANCELLED) {
@@ -2957,13 +2958,18 @@ static MigThrError migration_detect_error(MigrationState *s)
}
/* Try to detect any file errors */
- ret = qemu_file_get_error(s->to_dst_file);
-
+ ret = qemu_file_get_error_obj(s->to_dst_file, &local_error);
if (!ret) {
/* Everything is fine */
+ assert(!local_error);
return MIG_THR_ERR_NONE;
}
+ if (local_error) {
+ migrate_set_error(s, local_error);
+ error_free(local_error);
+ }
+
if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) {
/*
* For postcopy, we allow the network to be down for a
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 8e639eb496..c382ea2d78 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -33,7 +33,8 @@
static ssize_t channel_writev_buffer(void *opaque,
struct iovec *iov,
int iovcnt,
- int64_t pos)
+ int64_t pos,
+ Error **errp)
{
QIOChannel *ioc = QIO_CHANNEL(opaque);
ssize_t done = 0;
@@ -47,7 +48,7 @@ static ssize_t channel_writev_buffer(void *opaque,
while (nlocal_iov > 0) {
ssize_t len;
- len = qio_channel_writev(ioc, local_iov, nlocal_iov, NULL);
+ len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
if (len == QIO_CHANNEL_ERR_BLOCK) {
if (qemu_in_coroutine()) {
qio_channel_yield(ioc, G_IO_OUT);
@@ -57,7 +58,6 @@ static ssize_t channel_writev_buffer(void *opaque,
continue;
}
if (len < 0) {
- /* XXX handle Error objects */
done = -EIO;
goto cleanup;
}
@@ -75,13 +75,14 @@ static ssize_t channel_writev_buffer(void *opaque,
static ssize_t channel_get_buffer(void *opaque,
uint8_t *buf,
int64_t pos,
- size_t size)
+ size_t size,
+ Error **errp)
{
QIOChannel *ioc = QIO_CHANNEL(opaque);
ssize_t ret;
do {
- ret = qio_channel_read(ioc, (char *)buf, size, NULL);
+ ret = qio_channel_read(ioc, (char *)buf, size, errp);
if (ret < 0) {
if (ret == QIO_CHANNEL_ERR_BLOCK) {
if (qemu_in_coroutine()) {
@@ -90,7 +91,6 @@ static ssize_t channel_get_buffer(void *opaque,
qio_channel_wait(ioc, G_IO_IN);
}
} else {
- /* XXX handle Error * object */
return -EIO;
}
}
@@ -100,18 +100,20 @@ static ssize_t channel_get_buffer(void *opaque,
}
-static int channel_close(void *opaque)
+static int channel_close(void *opaque, Error **errp)
{
+ int ret;
QIOChannel *ioc = QIO_CHANNEL(opaque);
- qio_channel_close(ioc, NULL);
+ ret = qio_channel_close(ioc, errp);
object_unref(OBJECT(ioc));
- return 0;
+ return ret;
}
static int channel_shutdown(void *opaque,
bool rd,
- bool wr)
+ bool wr,
+ Error **errp)
{
QIOChannel *ioc = QIO_CHANNEL(opaque);
@@ -125,8 +127,7 @@ static int channel_shutdown(void *opaque,
} else {
mode = QIO_CHANNEL_SHUTDOWN_WRITE;
}
- if (qio_channel_shutdown(ioc, mode, NULL) < 0) {
- /* XXX handler Error * object */
+ if (qio_channel_shutdown(ioc, mode, errp) < 0) {
return -EIO;
}
}
@@ -135,11 +136,12 @@ static int channel_shutdown(void *opaque,
static int channel_set_blocking(void *opaque,
- bool enabled)
+ bool enabled,
+ Error **errp)
{
QIOChannel *ioc = QIO_CHANNEL(opaque);
- if (qio_channel_set_blocking(ioc, enabled, NULL) < 0) {
+ if (qio_channel_set_blocking(ioc, enabled, errp) < 0) {
return -1;
}
return 0;
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 977b9ae07c..c52160e08b 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -29,6 +29,7 @@
#include "migration.h"
#include "qemu-file.h"
#include "trace.h"
+#include "qapi/error.h"
#define IO_BUF_SIZE 32768
#define MAX_IOV_SIZE MIN(IOV_MAX, 64)
@@ -52,6 +53,7 @@ struct QEMUFile {
unsigned int iovcnt;
int last_error;
+ Error *last_error_obj;
};
/*
@@ -63,7 +65,7 @@ int qemu_file_shutdown(QEMUFile *f)
if (!f->ops->shut_down) {
return -ENOSYS;
}
- return f->ops->shut_down(f->opaque, true, true);
+ return f->ops->shut_down(f->opaque, true, true, NULL);
}
/*
@@ -108,24 +110,55 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
}
/*
- * Get last error for stream f
+ * Get last error for stream f with optional Error*
*
* Return negative error value if there has been an error on previous
* operations, return 0 if no error happened.
+ * Optional, it returns Error* in errp, but it may be NULL even if return value
+ * is not 0.
*
*/
-int qemu_file_get_error(QEMUFile *f)
+int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
{
+ if (errp) {
+ *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
+ }
return f->last_error;
}
-void qemu_file_set_error(QEMUFile *f, int ret)
+/*
+ * Set the last error for stream f with optional Error*
+ */
+void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err)
{
- if (f->last_error == 0) {
+ if (f->last_error == 0 && ret) {
f->last_error = ret;
+ error_propagate(&f->last_error_obj, err);
+ } else if (err) {
+ error_report_err(err);
}
}
+/*
+ * Get last error for stream f
+ *
+ * Return negative error value if there has been an error on previous
+ * operations, return 0 if no error happened.
+ *
+ */
+int qemu_file_get_error(QEMUFile *f)
+{
+ return qemu_file_get_error_obj(f, NULL);
+}
+
+/*
+ * Set the last error for stream f
+ */
+void qemu_file_set_error(QEMUFile *f, int ret)
+{
+ qemu_file_set_error_obj(f, ret, NULL);
+}
+
bool qemu_file_is_writable(QEMUFile *f)
{
return f->ops->writev_buffer;
@@ -177,6 +210,7 @@ void qemu_fflush(QEMUFile *f)
{
ssize_t ret = 0;
ssize_t expect = 0;
+ Error *local_error = NULL;
if (!qemu_file_is_writable(f)) {
return;
@@ -184,7 +218,8 @@ void qemu_fflush(QEMUFile *f)
if (f->iovcnt > 0) {
expect = iov_size(f->iov, f->iovcnt);
- ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos);
+ ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
+ &local_error);
qemu_iovec_release_ram(f);
}
@@ -196,7 +231,7 @@ void qemu_fflush(QEMUFile *f)
* data set we requested, so sanity check that.
*/
if (ret != expect) {
- qemu_file_set_error(f, ret < 0 ? ret : -EIO);
+ qemu_file_set_error_obj(f, ret < 0 ? ret : -EIO, local_error);
}
f->buf_index = 0;
f->iovcnt = 0;
@@ -284,6 +319,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
{
int len;
int pending;
+ Error *local_error = NULL;
assert(!qemu_file_is_writable(f));
@@ -295,14 +331,16 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
f->buf_size = pending;
len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
- IO_BUF_SIZE - pending);
+ IO_BUF_SIZE - pending, &local_error);
if (len > 0) {
f->buf_size += len;
f->pos += len;
} else if (len == 0) {
- qemu_file_set_error(f, -EIO);
+ qemu_file_set_error_obj(f, -EIO, local_error);
} else if (len != -EAGAIN) {
- qemu_file_set_error(f, len);
+ qemu_file_set_error_obj(f, len, local_error);
+ } else {
+ error_free(local_error);
}
return len;
@@ -328,7 +366,7 @@ int qemu_fclose(QEMUFile *f)
ret = qemu_file_get_error(f);
if (f->ops->close) {
- int ret2 = f->ops->close(f->opaque);
+ int ret2 = f->ops->close(f->opaque, NULL);
if (ret >= 0) {
ret = ret2;
}
@@ -339,6 +377,7 @@ int qemu_fclose(QEMUFile *f)
if (f->last_error) {
ret = f->last_error;
}
+ error_free(f->last_error_obj);
g_free(f);
trace_qemu_file_fclose();
return ret;
@@ -784,6 +823,6 @@ void qemu_put_counted_string(QEMUFile *f, const char *str)
void qemu_file_set_blocking(QEMUFile *f, bool block)
{
if (f->ops->set_blocking) {
- f->ops->set_blocking(f->opaque, block);
+ f->ops->set_blocking(f->opaque, block, NULL);
}
}
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 13baf896bd..eb886db65f 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -32,7 +32,8 @@
* bytes actually read should be returned.
*/
typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
- int64_t pos, size_t size);
+ int64_t pos, size_t size,
+ Error **errp);
/* Close a file
*
@@ -41,7 +42,7 @@ typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
* The meaning of return value on success depends on the specific back-end being
* used.
*/
-typedef int (QEMUFileCloseFunc)(void *opaque);
+typedef int (QEMUFileCloseFunc)(void *opaque, Error **errp);
/* Called to return the OS file descriptor associated to the QEMUFile.
*/
@@ -49,14 +50,15 @@ typedef int (QEMUFileGetFD)(void *opaque);
/* Called to change the blocking mode of the file
*/
-typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled);
+typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled, Error **errp);
/*
* This function writes an iovec to file. The handler must write all
* of the data or return a negative errno value.
*/
typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
- int iovcnt, int64_t pos);
+ int iovcnt, int64_t pos,
+ Error **errp);
/*
* This function provides hooks around different
@@ -97,7 +99,8 @@ typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
* Existing blocking reads/writes must be woken
* Returns 0 on success, -err on error
*/
-typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr);
+typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr,
+ Error **errp);
typedef struct QEMUFileOps {
QEMUFileGetBufferFunc *get_buffer;
@@ -149,6 +152,8 @@ void qemu_update_position(QEMUFile *f, size_t size);
void qemu_file_reset_rate_limit(QEMUFile *f);
void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
int64_t qemu_file_get_rate_limit(QEMUFile *f);
+int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
+void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
void qemu_file_set_error(QEMUFile *f, int ret);
int qemu_file_shutdown(QEMUFile *f);
QEMUFile *qemu_file_get_return_path(QEMUFile *f);
diff --git a/migration/savevm.c b/migration/savevm.c
index 34bcad3807..a619af744d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -124,7 +124,7 @@ static struct mig_cmd_args {
/* savevm/loadvm support */
static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
- int64_t pos)
+ int64_t pos, Error **errp)
{
int ret;
QEMUIOVector qiov;
@@ -139,12 +139,12 @@ static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
}
static ssize_t block_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
- size_t size)
+ size_t size, Error **errp)
{
return bdrv_load_vmstate(opaque, buf, pos, size);
}
-static int bdrv_fclose(void *opaque)
+static int bdrv_fclose(void *opaque, Error **errp)
{
return bdrv_flush(opaque);
}
--
2.21.0
Ping
22.04.2019, 13:50, "Yury Kotov" <yury-kotov@yandex-team.ru>:
> Currently, there is no information about error if outgoing migration was failed
> because of file channel errors.
> Example (QMP session):
> -> { "execute": "migrate", "arguments": { "uri": "exec:head -c 1" }}
> <- { "return": {} }
> ...
> -> { "execute": "query-migrate" }
> <- { "return": { "status": "failed" }} // There is not error's description
>
> And even in the QEMU's output there is nothing.
>
> This patch
> 1) Adds errp for the most of QEMUFileOps
> 2) Adds qemu_file_get_error_obj/qemu_file_set_error_obj
> 3) And finally using of qemu_file_get_error_obj in migration.c
>
> And now, the status for the mentioned fail will be:
> -> { "execute": "query-migrate" }
> <- { "return": { "status": "failed",
> "error-desc": "Unable to write to command: Broken pipe" }}
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
> migration/migration.c | 10 ++++--
> migration/qemu-file-channel.c | 30 +++++++++--------
> migration/qemu-file.c | 63 ++++++++++++++++++++++++++++-------
> migration/qemu-file.h | 15 ++++++---
> migration/savevm.c | 6 ++--
> 5 files changed, 88 insertions(+), 36 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 609e0df5d0..7bcdc4613b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2949,6 +2949,7 @@ static MigThrError migration_detect_error(MigrationState *s)
> {
> int ret;
> int state = s->state;
> + Error *local_error = NULL;
>
> if (state == MIGRATION_STATUS_CANCELLING ||
> state == MIGRATION_STATUS_CANCELLED) {
> @@ -2957,13 +2958,18 @@ static MigThrError migration_detect_error(MigrationState *s)
> }
>
> /* Try to detect any file errors */
> - ret = qemu_file_get_error(s->to_dst_file);
> -
> + ret = qemu_file_get_error_obj(s->to_dst_file, &local_error);
> if (!ret) {
> /* Everything is fine */
> + assert(!local_error);
> return MIG_THR_ERR_NONE;
> }
>
> + if (local_error) {
> + migrate_set_error(s, local_error);
> + error_free(local_error);
> + }
> +
> if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) {
> /*
> * For postcopy, we allow the network to be down for a
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 8e639eb496..c382ea2d78 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -33,7 +33,8 @@
> static ssize_t channel_writev_buffer(void *opaque,
> struct iovec *iov,
> int iovcnt,
> - int64_t pos)
> + int64_t pos,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ssize_t done = 0;
> @@ -47,7 +48,7 @@ static ssize_t channel_writev_buffer(void *opaque,
>
> while (nlocal_iov > 0) {
> ssize_t len;
> - len = qio_channel_writev(ioc, local_iov, nlocal_iov, NULL);
> + len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
> if (len == QIO_CHANNEL_ERR_BLOCK) {
> if (qemu_in_coroutine()) {
> qio_channel_yield(ioc, G_IO_OUT);
> @@ -57,7 +58,6 @@ static ssize_t channel_writev_buffer(void *opaque,
> continue;
> }
> if (len < 0) {
> - /* XXX handle Error objects */
> done = -EIO;
> goto cleanup;
> }
> @@ -75,13 +75,14 @@ static ssize_t channel_writev_buffer(void *opaque,
> static ssize_t channel_get_buffer(void *opaque,
> uint8_t *buf,
> int64_t pos,
> - size_t size)
> + size_t size,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ssize_t ret;
>
> do {
> - ret = qio_channel_read(ioc, (char *)buf, size, NULL);
> + ret = qio_channel_read(ioc, (char *)buf, size, errp);
> if (ret < 0) {
> if (ret == QIO_CHANNEL_ERR_BLOCK) {
> if (qemu_in_coroutine()) {
> @@ -90,7 +91,6 @@ static ssize_t channel_get_buffer(void *opaque,
> qio_channel_wait(ioc, G_IO_IN);
> }
> } else {
> - /* XXX handle Error * object */
> return -EIO;
> }
> }
> @@ -100,18 +100,20 @@ static ssize_t channel_get_buffer(void *opaque,
> }
>
> -static int channel_close(void *opaque)
> +static int channel_close(void *opaque, Error **errp)
> {
> + int ret;
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> - qio_channel_close(ioc, NULL);
> + ret = qio_channel_close(ioc, errp);
> object_unref(OBJECT(ioc));
> - return 0;
> + return ret;
> }
>
> static int channel_shutdown(void *opaque,
> bool rd,
> - bool wr)
> + bool wr,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
>
> @@ -125,8 +127,7 @@ static int channel_shutdown(void *opaque,
> } else {
> mode = QIO_CHANNEL_SHUTDOWN_WRITE;
> }
> - if (qio_channel_shutdown(ioc, mode, NULL) < 0) {
> - /* XXX handler Error * object */
> + if (qio_channel_shutdown(ioc, mode, errp) < 0) {
> return -EIO;
> }
> }
> @@ -135,11 +136,12 @@ static int channel_shutdown(void *opaque,
>
> static int channel_set_blocking(void *opaque,
> - bool enabled)
> + bool enabled,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
>
> - if (qio_channel_set_blocking(ioc, enabled, NULL) < 0) {
> + if (qio_channel_set_blocking(ioc, enabled, errp) < 0) {
> return -1;
> }
> return 0;
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 977b9ae07c..c52160e08b 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -29,6 +29,7 @@
> #include "migration.h"
> #include "qemu-file.h"
> #include "trace.h"
> +#include "qapi/error.h"
>
> #define IO_BUF_SIZE 32768
> #define MAX_IOV_SIZE MIN(IOV_MAX, 64)
> @@ -52,6 +53,7 @@ struct QEMUFile {
> unsigned int iovcnt;
>
> int last_error;
> + Error *last_error_obj;
> };
>
> /*
> @@ -63,7 +65,7 @@ int qemu_file_shutdown(QEMUFile *f)
> if (!f->ops->shut_down) {
> return -ENOSYS;
> }
> - return f->ops->shut_down(f->opaque, true, true);
> + return f->ops->shut_down(f->opaque, true, true, NULL);
> }
>
> /*
> @@ -108,24 +110,55 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
> }
>
> /*
> - * Get last error for stream f
> + * Get last error for stream f with optional Error*
> *
> * Return negative error value if there has been an error on previous
> * operations, return 0 if no error happened.
> + * Optional, it returns Error* in errp, but it may be NULL even if return value
> + * is not 0.
> *
> */
> -int qemu_file_get_error(QEMUFile *f)
> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
> {
> + if (errp) {
> + *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
> + }
> return f->last_error;
> }
>
> -void qemu_file_set_error(QEMUFile *f, int ret)
> +/*
> + * Set the last error for stream f with optional Error*
> + */
> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err)
> {
> - if (f->last_error == 0) {
> + if (f->last_error == 0 && ret) {
> f->last_error = ret;
> + error_propagate(&f->last_error_obj, err);
> + } else if (err) {
> + error_report_err(err);
> }
> }
>
> +/*
> + * Get last error for stream f
> + *
> + * Return negative error value if there has been an error on previous
> + * operations, return 0 if no error happened.
> + *
> + */
> +int qemu_file_get_error(QEMUFile *f)
> +{
> + return qemu_file_get_error_obj(f, NULL);
> +}
> +
> +/*
> + * Set the last error for stream f
> + */
> +void qemu_file_set_error(QEMUFile *f, int ret)
> +{
> + qemu_file_set_error_obj(f, ret, NULL);
> +}
> +
> bool qemu_file_is_writable(QEMUFile *f)
> {
> return f->ops->writev_buffer;
> @@ -177,6 +210,7 @@ void qemu_fflush(QEMUFile *f)
> {
> ssize_t ret = 0;
> ssize_t expect = 0;
> + Error *local_error = NULL;
>
> if (!qemu_file_is_writable(f)) {
> return;
> @@ -184,7 +218,8 @@ void qemu_fflush(QEMUFile *f)
>
> if (f->iovcnt > 0) {
> expect = iov_size(f->iov, f->iovcnt);
> - ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos);
> + ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
> + &local_error);
>
> qemu_iovec_release_ram(f);
> }
> @@ -196,7 +231,7 @@ void qemu_fflush(QEMUFile *f)
> * data set we requested, so sanity check that.
> */
> if (ret != expect) {
> - qemu_file_set_error(f, ret < 0 ? ret : -EIO);
> + qemu_file_set_error_obj(f, ret < 0 ? ret : -EIO, local_error);
> }
> f->buf_index = 0;
> f->iovcnt = 0;
> @@ -284,6 +319,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> {
> int len;
> int pending;
> + Error *local_error = NULL;
>
> assert(!qemu_file_is_writable(f));
>
> @@ -295,14 +331,16 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> f->buf_size = pending;
>
> len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
> - IO_BUF_SIZE - pending);
> + IO_BUF_SIZE - pending, &local_error);
> if (len > 0) {
> f->buf_size += len;
> f->pos += len;
> } else if (len == 0) {
> - qemu_file_set_error(f, -EIO);
> + qemu_file_set_error_obj(f, -EIO, local_error);
> } else if (len != -EAGAIN) {
> - qemu_file_set_error(f, len);
> + qemu_file_set_error_obj(f, len, local_error);
> + } else {
> + error_free(local_error);
> }
>
> return len;
> @@ -328,7 +366,7 @@ int qemu_fclose(QEMUFile *f)
> ret = qemu_file_get_error(f);
>
> if (f->ops->close) {
> - int ret2 = f->ops->close(f->opaque);
> + int ret2 = f->ops->close(f->opaque, NULL);
> if (ret >= 0) {
> ret = ret2;
> }
> @@ -339,6 +377,7 @@ int qemu_fclose(QEMUFile *f)
> if (f->last_error) {
> ret = f->last_error;
> }
> + error_free(f->last_error_obj);
> g_free(f);
> trace_qemu_file_fclose();
> return ret;
> @@ -784,6 +823,6 @@ void qemu_put_counted_string(QEMUFile *f, const char *str)
> void qemu_file_set_blocking(QEMUFile *f, bool block)
> {
> if (f->ops->set_blocking) {
> - f->ops->set_blocking(f->opaque, block);
> + f->ops->set_blocking(f->opaque, block, NULL);
> }
> }
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 13baf896bd..eb886db65f 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -32,7 +32,8 @@
> * bytes actually read should be returned.
> */
> typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> - int64_t pos, size_t size);
> + int64_t pos, size_t size,
> + Error **errp);
>
> /* Close a file
> *
> @@ -41,7 +42,7 @@ typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> * The meaning of return value on success depends on the specific back-end being
> * used.
> */
> -typedef int (QEMUFileCloseFunc)(void *opaque);
> +typedef int (QEMUFileCloseFunc)(void *opaque, Error **errp);
>
> /* Called to return the OS file descriptor associated to the QEMUFile.
> */
> @@ -49,14 +50,15 @@ typedef int (QEMUFileGetFD)(void *opaque);
>
> /* Called to change the blocking mode of the file
> */
> -typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled);
> +typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled, Error **errp);
>
> /*
> * This function writes an iovec to file. The handler must write all
> * of the data or return a negative errno value.
> */
> typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
> - int iovcnt, int64_t pos);
> + int iovcnt, int64_t pos,
> + Error **errp);
>
> /*
> * This function provides hooks around different
> @@ -97,7 +99,8 @@ typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
> * Existing blocking reads/writes must be woken
> * Returns 0 on success, -err on error
> */
> -typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr);
> +typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr,
> + Error **errp);
>
> typedef struct QEMUFileOps {
> QEMUFileGetBufferFunc *get_buffer;
> @@ -149,6 +152,8 @@ void qemu_update_position(QEMUFile *f, size_t size);
> void qemu_file_reset_rate_limit(QEMUFile *f);
> void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
> int64_t qemu_file_get_rate_limit(QEMUFile *f);
> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
> void qemu_file_set_error(QEMUFile *f, int ret);
> int qemu_file_shutdown(QEMUFile *f);
> QEMUFile *qemu_file_get_return_path(QEMUFile *f);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 34bcad3807..a619af744d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -124,7 +124,7 @@ static struct mig_cmd_args {
> /* savevm/loadvm support */
>
> static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> - int64_t pos)
> + int64_t pos, Error **errp)
> {
> int ret;
> QEMUIOVector qiov;
> @@ -139,12 +139,12 @@ static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> }
>
> static ssize_t block_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
> - size_t size)
> + size_t size, Error **errp)
> {
> return bdrv_load_vmstate(opaque, buf, pos, size);
> }
>
> -static int bdrv_fclose(void *opaque)
> +static int bdrv_fclose(void *opaque, Error **errp)
> {
> return bdrv_flush(opaque);
> }
> --
> 2.21.0
* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> Currently, there is no information about error if outgoing migration was failed
> because of file channel errors.
> Example (QMP session):
> -> { "execute": "migrate", "arguments": { "uri": "exec:head -c 1" }}
> <- { "return": {} }
> ...
> -> { "execute": "query-migrate" }
> <- { "return": { "status": "failed" }} // There is not error's description
>
> And even in the QEMU's output there is nothing.
>
> This patch
> 1) Adds errp for the most of QEMUFileOps
> 2) Adds qemu_file_get_error_obj/qemu_file_set_error_obj
> 3) And finally using of qemu_file_get_error_obj in migration.c
>
> And now, the status for the mentioned fail will be:
> -> { "execute": "query-migrate" }
> <- { "return": { "status": "failed",
> "error-desc": "Unable to write to command: Broken pipe" }}
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/migration.c | 10 ++++--
> migration/qemu-file-channel.c | 30 +++++++++--------
> migration/qemu-file.c | 63 ++++++++++++++++++++++++++++-------
> migration/qemu-file.h | 15 ++++++---
> migration/savevm.c | 6 ++--
> 5 files changed, 88 insertions(+), 36 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 609e0df5d0..7bcdc4613b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2949,6 +2949,7 @@ static MigThrError migration_detect_error(MigrationState *s)
> {
> int ret;
> int state = s->state;
> + Error *local_error = NULL;
>
> if (state == MIGRATION_STATUS_CANCELLING ||
> state == MIGRATION_STATUS_CANCELLED) {
> @@ -2957,13 +2958,18 @@ static MigThrError migration_detect_error(MigrationState *s)
> }
>
> /* Try to detect any file errors */
> - ret = qemu_file_get_error(s->to_dst_file);
> -
> + ret = qemu_file_get_error_obj(s->to_dst_file, &local_error);
> if (!ret) {
> /* Everything is fine */
> + assert(!local_error);
> return MIG_THR_ERR_NONE;
> }
>
> + if (local_error) {
> + migrate_set_error(s, local_error);
> + error_free(local_error);
> + }
> +
> if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) {
> /*
> * For postcopy, we allow the network to be down for a
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 8e639eb496..c382ea2d78 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -33,7 +33,8 @@
> static ssize_t channel_writev_buffer(void *opaque,
> struct iovec *iov,
> int iovcnt,
> - int64_t pos)
> + int64_t pos,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ssize_t done = 0;
> @@ -47,7 +48,7 @@ static ssize_t channel_writev_buffer(void *opaque,
>
> while (nlocal_iov > 0) {
> ssize_t len;
> - len = qio_channel_writev(ioc, local_iov, nlocal_iov, NULL);
> + len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
> if (len == QIO_CHANNEL_ERR_BLOCK) {
> if (qemu_in_coroutine()) {
> qio_channel_yield(ioc, G_IO_OUT);
> @@ -57,7 +58,6 @@ static ssize_t channel_writev_buffer(void *opaque,
> continue;
> }
> if (len < 0) {
> - /* XXX handle Error objects */
> done = -EIO;
> goto cleanup;
> }
> @@ -75,13 +75,14 @@ static ssize_t channel_writev_buffer(void *opaque,
> static ssize_t channel_get_buffer(void *opaque,
> uint8_t *buf,
> int64_t pos,
> - size_t size)
> + size_t size,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ssize_t ret;
>
> do {
> - ret = qio_channel_read(ioc, (char *)buf, size, NULL);
> + ret = qio_channel_read(ioc, (char *)buf, size, errp);
> if (ret < 0) {
> if (ret == QIO_CHANNEL_ERR_BLOCK) {
> if (qemu_in_coroutine()) {
> @@ -90,7 +91,6 @@ static ssize_t channel_get_buffer(void *opaque,
> qio_channel_wait(ioc, G_IO_IN);
> }
> } else {
> - /* XXX handle Error * object */
> return -EIO;
> }
> }
> @@ -100,18 +100,20 @@ static ssize_t channel_get_buffer(void *opaque,
> }
>
>
> -static int channel_close(void *opaque)
> +static int channel_close(void *opaque, Error **errp)
> {
> + int ret;
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> - qio_channel_close(ioc, NULL);
> + ret = qio_channel_close(ioc, errp);
> object_unref(OBJECT(ioc));
> - return 0;
> + return ret;
> }
>
>
> static int channel_shutdown(void *opaque,
> bool rd,
> - bool wr)
> + bool wr,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
>
> @@ -125,8 +127,7 @@ static int channel_shutdown(void *opaque,
> } else {
> mode = QIO_CHANNEL_SHUTDOWN_WRITE;
> }
> - if (qio_channel_shutdown(ioc, mode, NULL) < 0) {
> - /* XXX handler Error * object */
> + if (qio_channel_shutdown(ioc, mode, errp) < 0) {
> return -EIO;
> }
> }
> @@ -135,11 +136,12 @@ static int channel_shutdown(void *opaque,
>
>
> static int channel_set_blocking(void *opaque,
> - bool enabled)
> + bool enabled,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
>
> - if (qio_channel_set_blocking(ioc, enabled, NULL) < 0) {
> + if (qio_channel_set_blocking(ioc, enabled, errp) < 0) {
> return -1;
> }
> return 0;
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 977b9ae07c..c52160e08b 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -29,6 +29,7 @@
> #include "migration.h"
> #include "qemu-file.h"
> #include "trace.h"
> +#include "qapi/error.h"
>
> #define IO_BUF_SIZE 32768
> #define MAX_IOV_SIZE MIN(IOV_MAX, 64)
> @@ -52,6 +53,7 @@ struct QEMUFile {
> unsigned int iovcnt;
>
> int last_error;
> + Error *last_error_obj;
> };
>
> /*
> @@ -63,7 +65,7 @@ int qemu_file_shutdown(QEMUFile *f)
> if (!f->ops->shut_down) {
> return -ENOSYS;
> }
> - return f->ops->shut_down(f->opaque, true, true);
> + return f->ops->shut_down(f->opaque, true, true, NULL);
> }
>
> /*
> @@ -108,24 +110,55 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
> }
>
> /*
> - * Get last error for stream f
> + * Get last error for stream f with optional Error*
> *
> * Return negative error value if there has been an error on previous
> * operations, return 0 if no error happened.
> + * Optional, it returns Error* in errp, but it may be NULL even if return value
> + * is not 0.
> *
> */
> -int qemu_file_get_error(QEMUFile *f)
> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
> {
> + if (errp) {
> + *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
> + }
> return f->last_error;
> }
>
> -void qemu_file_set_error(QEMUFile *f, int ret)
> +/*
> + * Set the last error for stream f with optional Error*
> + */
> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err)
> {
> - if (f->last_error == 0) {
> + if (f->last_error == 0 && ret) {
> f->last_error = ret;
> + error_propagate(&f->last_error_obj, err);
> + } else if (err) {
> + error_report_err(err);
> }
> }
>
> +/*
> + * Get last error for stream f
> + *
> + * Return negative error value if there has been an error on previous
> + * operations, return 0 if no error happened.
> + *
> + */
> +int qemu_file_get_error(QEMUFile *f)
> +{
> + return qemu_file_get_error_obj(f, NULL);
> +}
> +
> +/*
> + * Set the last error for stream f
> + */
> +void qemu_file_set_error(QEMUFile *f, int ret)
> +{
> + qemu_file_set_error_obj(f, ret, NULL);
> +}
> +
> bool qemu_file_is_writable(QEMUFile *f)
> {
> return f->ops->writev_buffer;
> @@ -177,6 +210,7 @@ void qemu_fflush(QEMUFile *f)
> {
> ssize_t ret = 0;
> ssize_t expect = 0;
> + Error *local_error = NULL;
>
> if (!qemu_file_is_writable(f)) {
> return;
> @@ -184,7 +218,8 @@ void qemu_fflush(QEMUFile *f)
>
> if (f->iovcnt > 0) {
> expect = iov_size(f->iov, f->iovcnt);
> - ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos);
> + ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
> + &local_error);
>
> qemu_iovec_release_ram(f);
> }
> @@ -196,7 +231,7 @@ void qemu_fflush(QEMUFile *f)
> * data set we requested, so sanity check that.
> */
> if (ret != expect) {
> - qemu_file_set_error(f, ret < 0 ? ret : -EIO);
> + qemu_file_set_error_obj(f, ret < 0 ? ret : -EIO, local_error);
> }
> f->buf_index = 0;
> f->iovcnt = 0;
> @@ -284,6 +319,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> {
> int len;
> int pending;
> + Error *local_error = NULL;
>
> assert(!qemu_file_is_writable(f));
>
> @@ -295,14 +331,16 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> f->buf_size = pending;
>
> len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
> - IO_BUF_SIZE - pending);
> + IO_BUF_SIZE - pending, &local_error);
> if (len > 0) {
> f->buf_size += len;
> f->pos += len;
> } else if (len == 0) {
> - qemu_file_set_error(f, -EIO);
> + qemu_file_set_error_obj(f, -EIO, local_error);
> } else if (len != -EAGAIN) {
> - qemu_file_set_error(f, len);
> + qemu_file_set_error_obj(f, len, local_error);
> + } else {
> + error_free(local_error);
> }
>
> return len;
> @@ -328,7 +366,7 @@ int qemu_fclose(QEMUFile *f)
> ret = qemu_file_get_error(f);
>
> if (f->ops->close) {
> - int ret2 = f->ops->close(f->opaque);
> + int ret2 = f->ops->close(f->opaque, NULL);
> if (ret >= 0) {
> ret = ret2;
> }
> @@ -339,6 +377,7 @@ int qemu_fclose(QEMUFile *f)
> if (f->last_error) {
> ret = f->last_error;
> }
> + error_free(f->last_error_obj);
> g_free(f);
> trace_qemu_file_fclose();
> return ret;
> @@ -784,6 +823,6 @@ void qemu_put_counted_string(QEMUFile *f, const char *str)
> void qemu_file_set_blocking(QEMUFile *f, bool block)
> {
> if (f->ops->set_blocking) {
> - f->ops->set_blocking(f->opaque, block);
> + f->ops->set_blocking(f->opaque, block, NULL);
> }
> }
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 13baf896bd..eb886db65f 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -32,7 +32,8 @@
> * bytes actually read should be returned.
> */
> typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> - int64_t pos, size_t size);
> + int64_t pos, size_t size,
> + Error **errp);
>
> /* Close a file
> *
> @@ -41,7 +42,7 @@ typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> * The meaning of return value on success depends on the specific back-end being
> * used.
> */
> -typedef int (QEMUFileCloseFunc)(void *opaque);
> +typedef int (QEMUFileCloseFunc)(void *opaque, Error **errp);
>
> /* Called to return the OS file descriptor associated to the QEMUFile.
> */
> @@ -49,14 +50,15 @@ typedef int (QEMUFileGetFD)(void *opaque);
>
> /* Called to change the blocking mode of the file
> */
> -typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled);
> +typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled, Error **errp);
>
> /*
> * This function writes an iovec to file. The handler must write all
> * of the data or return a negative errno value.
> */
> typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
> - int iovcnt, int64_t pos);
> + int iovcnt, int64_t pos,
> + Error **errp);
>
> /*
> * This function provides hooks around different
> @@ -97,7 +99,8 @@ typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
> * Existing blocking reads/writes must be woken
> * Returns 0 on success, -err on error
> */
> -typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr);
> +typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr,
> + Error **errp);
>
> typedef struct QEMUFileOps {
> QEMUFileGetBufferFunc *get_buffer;
> @@ -149,6 +152,8 @@ void qemu_update_position(QEMUFile *f, size_t size);
> void qemu_file_reset_rate_limit(QEMUFile *f);
> void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
> int64_t qemu_file_get_rate_limit(QEMUFile *f);
> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
> void qemu_file_set_error(QEMUFile *f, int ret);
> int qemu_file_shutdown(QEMUFile *f);
> QEMUFile *qemu_file_get_return_path(QEMUFile *f);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 34bcad3807..a619af744d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -124,7 +124,7 @@ static struct mig_cmd_args {
> /* savevm/loadvm support */
>
> static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> - int64_t pos)
> + int64_t pos, Error **errp)
> {
> int ret;
> QEMUIOVector qiov;
> @@ -139,12 +139,12 @@ static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> }
>
> static ssize_t block_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
> - size_t size)
> + size_t size, Error **errp)
> {
> return bdrv_load_vmstate(opaque, buf, pos, size);
> }
>
> -static int bdrv_fclose(void *opaque)
> +static int bdrv_fclose(void *opaque, Error **errp)
> {
> return bdrv_flush(opaque);
> }
> --
> 2.21.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Hi, I'm a bit worried that this patch might have been forgotten.
Is it queued? Thanks!
14.06.2019, 19:56, "Dr. David Alan Gilbert" <dgilbert@redhat.com>:
> * Yury Kotov (yury-kotov@yandex-team.ru) wrote:
>> Currently, there is no information about error if outgoing migration was failed
>> because of file channel errors.
>> Example (QMP session):
>> -> { "execute": "migrate", "arguments": { "uri": "exec:head -c 1" }}
>> <- { "return": {} }
>> ...
>> -> { "execute": "query-migrate" }
>> <- { "return": { "status": "failed" }} // There is not error's description
>>
>> And even in the QEMU's output there is nothing.
>>
>> This patch
>> 1) Adds errp for the most of QEMUFileOps
>> 2) Adds qemu_file_get_error_obj/qemu_file_set_error_obj
>> 3) And finally using of qemu_file_get_error_obj in migration.c
>>
>> And now, the status for the mentioned fail will be:
>> -> { "execute": "query-migrate" }
>> <- { "return": { "status": "failed",
>> "error-desc": "Unable to write to command: Broken pipe" }}
>>
>> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>> ---
>> migration/migration.c | 10 ++++--
>> migration/qemu-file-channel.c | 30 +++++++++--------
>> migration/qemu-file.c | 63 ++++++++++++++++++++++++++++-------
>> migration/qemu-file.h | 15 ++++++---
>> migration/savevm.c | 6 ++--
>> 5 files changed, 88 insertions(+), 36 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 609e0df5d0..7bcdc4613b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2949,6 +2949,7 @@ static MigThrError migration_detect_error(MigrationState *s)
>> {
>> int ret;
>> int state = s->state;
>> + Error *local_error = NULL;
>>
>> if (state == MIGRATION_STATUS_CANCELLING ||
>> state == MIGRATION_STATUS_CANCELLED) {
>> @@ -2957,13 +2958,18 @@ static MigThrError migration_detect_error(MigrationState *s)
>> }
>>
>> /* Try to detect any file errors */
>> - ret = qemu_file_get_error(s->to_dst_file);
>> -
>> + ret = qemu_file_get_error_obj(s->to_dst_file, &local_error);
>> if (!ret) {
>> /* Everything is fine */
>> + assert(!local_error);
>> return MIG_THR_ERR_NONE;
>> }
>>
>> + if (local_error) {
>> + migrate_set_error(s, local_error);
>> + error_free(local_error);
>> + }
>> +
>> if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) {
>> /*
>> * For postcopy, we allow the network to be down for a
>> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
>> index 8e639eb496..c382ea2d78 100644
>> --- a/migration/qemu-file-channel.c
>> +++ b/migration/qemu-file-channel.c
>> @@ -33,7 +33,8 @@
>> static ssize_t channel_writev_buffer(void *opaque,
>> struct iovec *iov,
>> int iovcnt,
>> - int64_t pos)
>> + int64_t pos,
>> + Error **errp)
>> {
>> QIOChannel *ioc = QIO_CHANNEL(opaque);
>> ssize_t done = 0;
>> @@ -47,7 +48,7 @@ static ssize_t channel_writev_buffer(void *opaque,
>>
>> while (nlocal_iov > 0) {
>> ssize_t len;
>> - len = qio_channel_writev(ioc, local_iov, nlocal_iov, NULL);
>> + len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
>> if (len == QIO_CHANNEL_ERR_BLOCK) {
>> if (qemu_in_coroutine()) {
>> qio_channel_yield(ioc, G_IO_OUT);
>> @@ -57,7 +58,6 @@ static ssize_t channel_writev_buffer(void *opaque,
>> continue;
>> }
>> if (len < 0) {
>> - /* XXX handle Error objects */
>> done = -EIO;
>> goto cleanup;
>> }
>> @@ -75,13 +75,14 @@ static ssize_t channel_writev_buffer(void *opaque,
>> static ssize_t channel_get_buffer(void *opaque,
>> uint8_t *buf,
>> int64_t pos,
>> - size_t size)
>> + size_t size,
>> + Error **errp)
>> {
>> QIOChannel *ioc = QIO_CHANNEL(opaque);
>> ssize_t ret;
>>
>> do {
>> - ret = qio_channel_read(ioc, (char *)buf, size, NULL);
>> + ret = qio_channel_read(ioc, (char *)buf, size, errp);
>> if (ret < 0) {
>> if (ret == QIO_CHANNEL_ERR_BLOCK) {
>> if (qemu_in_coroutine()) {
>> @@ -90,7 +91,6 @@ static ssize_t channel_get_buffer(void *opaque,
>> qio_channel_wait(ioc, G_IO_IN);
>> }
>> } else {
>> - /* XXX handle Error * object */
>> return -EIO;
>> }
>> }
>> @@ -100,18 +100,20 @@ static ssize_t channel_get_buffer(void *opaque,
>> }
>>
>> -static int channel_close(void *opaque)
>> +static int channel_close(void *opaque, Error **errp)
>> {
>> + int ret;
>> QIOChannel *ioc = QIO_CHANNEL(opaque);
>> - qio_channel_close(ioc, NULL);
>> + ret = qio_channel_close(ioc, errp);
>> object_unref(OBJECT(ioc));
>> - return 0;
>> + return ret;
>> }
>>
>> static int channel_shutdown(void *opaque,
>> bool rd,
>> - bool wr)
>> + bool wr,
>> + Error **errp)
>> {
>> QIOChannel *ioc = QIO_CHANNEL(opaque);
>>
>> @@ -125,8 +127,7 @@ static int channel_shutdown(void *opaque,
>> } else {
>> mode = QIO_CHANNEL_SHUTDOWN_WRITE;
>> }
>> - if (qio_channel_shutdown(ioc, mode, NULL) < 0) {
>> - /* XXX handler Error * object */
>> + if (qio_channel_shutdown(ioc, mode, errp) < 0) {
>> return -EIO;
>> }
>> }
>> @@ -135,11 +136,12 @@ static int channel_shutdown(void *opaque,
>>
>> static int channel_set_blocking(void *opaque,
>> - bool enabled)
>> + bool enabled,
>> + Error **errp)
>> {
>> QIOChannel *ioc = QIO_CHANNEL(opaque);
>>
>> - if (qio_channel_set_blocking(ioc, enabled, NULL) < 0) {
>> + if (qio_channel_set_blocking(ioc, enabled, errp) < 0) {
>> return -1;
>> }
>> return 0;
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index 977b9ae07c..c52160e08b 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -29,6 +29,7 @@
>> #include "migration.h"
>> #include "qemu-file.h"
>> #include "trace.h"
>> +#include "qapi/error.h"
>>
>> #define IO_BUF_SIZE 32768
>> #define MAX_IOV_SIZE MIN(IOV_MAX, 64)
>> @@ -52,6 +53,7 @@ struct QEMUFile {
>> unsigned int iovcnt;
>>
>> int last_error;
>> + Error *last_error_obj;
>> };
>>
>> /*
>> @@ -63,7 +65,7 @@ int qemu_file_shutdown(QEMUFile *f)
>> if (!f->ops->shut_down) {
>> return -ENOSYS;
>> }
>> - return f->ops->shut_down(f->opaque, true, true);
>> + return f->ops->shut_down(f->opaque, true, true, NULL);
>> }
>>
>> /*
>> @@ -108,24 +110,55 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
>> }
>>
>> /*
>> - * Get last error for stream f
>> + * Get last error for stream f with optional Error*
>> *
>> * Return negative error value if there has been an error on previous
>> * operations, return 0 if no error happened.
>> + * Optional, it returns Error* in errp, but it may be NULL even if return value
>> + * is not 0.
>> *
>> */
>> -int qemu_file_get_error(QEMUFile *f)
>> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
>> {
>> + if (errp) {
>> + *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
>> + }
>> return f->last_error;
>> }
>>
>> -void qemu_file_set_error(QEMUFile *f, int ret)
>> +/*
>> + * Set the last error for stream f with optional Error*
>> + */
>> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err)
>> {
>> - if (f->last_error == 0) {
>> + if (f->last_error == 0 && ret) {
>> f->last_error = ret;
>> + error_propagate(&f->last_error_obj, err);
>> + } else if (err) {
>> + error_report_err(err);
>> }
>> }
>>
>> +/*
>> + * Get last error for stream f
>> + *
>> + * Return negative error value if there has been an error on previous
>> + * operations, return 0 if no error happened.
>> + *
>> + */
>> +int qemu_file_get_error(QEMUFile *f)
>> +{
>> + return qemu_file_get_error_obj(f, NULL);
>> +}
>> +
>> +/*
>> + * Set the last error for stream f
>> + */
>> +void qemu_file_set_error(QEMUFile *f, int ret)
>> +{
>> + qemu_file_set_error_obj(f, ret, NULL);
>> +}
>> +
>> bool qemu_file_is_writable(QEMUFile *f)
>> {
>> return f->ops->writev_buffer;
>> @@ -177,6 +210,7 @@ void qemu_fflush(QEMUFile *f)
>> {
>> ssize_t ret = 0;
>> ssize_t expect = 0;
>> + Error *local_error = NULL;
>>
>> if (!qemu_file_is_writable(f)) {
>> return;
>> @@ -184,7 +218,8 @@ void qemu_fflush(QEMUFile *f)
>>
>> if (f->iovcnt > 0) {
>> expect = iov_size(f->iov, f->iovcnt);
>> - ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos);
>> + ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
>> + &local_error);
>>
>> qemu_iovec_release_ram(f);
>> }
>> @@ -196,7 +231,7 @@ void qemu_fflush(QEMUFile *f)
>> * data set we requested, so sanity check that.
>> */
>> if (ret != expect) {
>> - qemu_file_set_error(f, ret < 0 ? ret : -EIO);
>> + qemu_file_set_error_obj(f, ret < 0 ? ret : -EIO, local_error);
>> }
>> f->buf_index = 0;
>> f->iovcnt = 0;
>> @@ -284,6 +319,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
>> {
>> int len;
>> int pending;
>> + Error *local_error = NULL;
>>
>> assert(!qemu_file_is_writable(f));
>>
>> @@ -295,14 +331,16 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
>> f->buf_size = pending;
>>
>> len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
>> - IO_BUF_SIZE - pending);
>> + IO_BUF_SIZE - pending, &local_error);
>> if (len > 0) {
>> f->buf_size += len;
>> f->pos += len;
>> } else if (len == 0) {
>> - qemu_file_set_error(f, -EIO);
>> + qemu_file_set_error_obj(f, -EIO, local_error);
>> } else if (len != -EAGAIN) {
>> - qemu_file_set_error(f, len);
>> + qemu_file_set_error_obj(f, len, local_error);
>> + } else {
>> + error_free(local_error);
>> }
>>
>> return len;
>> @@ -328,7 +366,7 @@ int qemu_fclose(QEMUFile *f)
>> ret = qemu_file_get_error(f);
>>
>> if (f->ops->close) {
>> - int ret2 = f->ops->close(f->opaque);
>> + int ret2 = f->ops->close(f->opaque, NULL);
>> if (ret >= 0) {
>> ret = ret2;
>> }
>> @@ -339,6 +377,7 @@ int qemu_fclose(QEMUFile *f)
>> if (f->last_error) {
>> ret = f->last_error;
>> }
>> + error_free(f->last_error_obj);
>> g_free(f);
>> trace_qemu_file_fclose();
>> return ret;
>> @@ -784,6 +823,6 @@ void qemu_put_counted_string(QEMUFile *f, const char *str)
>> void qemu_file_set_blocking(QEMUFile *f, bool block)
>> {
>> if (f->ops->set_blocking) {
>> - f->ops->set_blocking(f->opaque, block);
>> + f->ops->set_blocking(f->opaque, block, NULL);
>> }
>> }
>> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
>> index 13baf896bd..eb886db65f 100644
>> --- a/migration/qemu-file.h
>> +++ b/migration/qemu-file.h
>> @@ -32,7 +32,8 @@
>> * bytes actually read should be returned.
>> */
>> typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
>> - int64_t pos, size_t size);
>> + int64_t pos, size_t size,
>> + Error **errp);
>>
>> /* Close a file
>> *
>> @@ -41,7 +42,7 @@ typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
>> * The meaning of return value on success depends on the specific back-end being
>> * used.
>> */
>> -typedef int (QEMUFileCloseFunc)(void *opaque);
>> +typedef int (QEMUFileCloseFunc)(void *opaque, Error **errp);
>>
>> /* Called to return the OS file descriptor associated to the QEMUFile.
>> */
>> @@ -49,14 +50,15 @@ typedef int (QEMUFileGetFD)(void *opaque);
>>
>> /* Called to change the blocking mode of the file
>> */
>> -typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled);
>> +typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled, Error **errp);
>>
>> /*
>> * This function writes an iovec to file. The handler must write all
>> * of the data or return a negative errno value.
>> */
>> typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
>> - int iovcnt, int64_t pos);
>> + int iovcnt, int64_t pos,
>> + Error **errp);
>>
>> /*
>> * This function provides hooks around different
>> @@ -97,7 +99,8 @@ typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
>> * Existing blocking reads/writes must be woken
>> * Returns 0 on success, -err on error
>> */
>> -typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr);
>> +typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr,
>> + Error **errp);
>>
>> typedef struct QEMUFileOps {
>> QEMUFileGetBufferFunc *get_buffer;
>> @@ -149,6 +152,8 @@ void qemu_update_position(QEMUFile *f, size_t size);
>> void qemu_file_reset_rate_limit(QEMUFile *f);
>> void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
>> int64_t qemu_file_get_rate_limit(QEMUFile *f);
>> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
>> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
>> void qemu_file_set_error(QEMUFile *f, int ret);
>> int qemu_file_shutdown(QEMUFile *f);
>> QEMUFile *qemu_file_get_return_path(QEMUFile *f);
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 34bcad3807..a619af744d 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -124,7 +124,7 @@ static struct mig_cmd_args {
>> /* savevm/loadvm support */
>>
>> static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
>> - int64_t pos)
>> + int64_t pos, Error **errp)
>> {
>> int ret;
>> QEMUIOVector qiov;
>> @@ -139,12 +139,12 @@ static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
>> }
>>
>> static ssize_t block_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
>> - size_t size)
>> + size_t size, Error **errp)
>> {
>> return bdrv_load_vmstate(opaque, buf, pos, size);
>> }
>>
>> -static int bdrv_fclose(void *opaque)
>> +static int bdrv_fclose(void *opaque, Error **errp)
>> {
>> return bdrv_flush(opaque);
>> }
>> --
>> 2.21.0
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Regards,
Yury
* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> Hi, I'm a bit worried that this patch might have been forgotten.
> Is it queued? Thanks!
I've added it to my list for the pull I'll do as soon as 4.2 opens.
Dave
> 14.06.2019, 19:56, "Dr. David Alan Gilbert" <dgilbert@redhat.com>:
> > * Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> >> Currently, there is no information about error if outgoing migration was failed
> >> because of file channel errors.
> >> Example (QMP session):
> >> -> { "execute": "migrate", "arguments": { "uri": "exec:head -c 1" }}
> >> <- { "return": {} }
> >> ...
> >> -> { "execute": "query-migrate" }
> >> <- { "return": { "status": "failed" }} // There is not error's description
> >>
> >> And even in the QEMU's output there is nothing.
> >>
> >> This patch
> >> 1) Adds errp for the most of QEMUFileOps
> >> 2) Adds qemu_file_get_error_obj/qemu_file_set_error_obj
> >> 3) And finally using of qemu_file_get_error_obj in migration.c
> >>
> >> And now, the status for the mentioned fail will be:
> >> -> { "execute": "query-migrate" }
> >> <- { "return": { "status": "failed",
> >> "error-desc": "Unable to write to command: Broken pipe" }}
> >>
> >> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> >
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> >> ---
> >> migration/migration.c | 10 ++++--
> >> migration/qemu-file-channel.c | 30 +++++++++--------
> >> migration/qemu-file.c | 63 ++++++++++++++++++++++++++++-------
> >> migration/qemu-file.h | 15 ++++++---
> >> migration/savevm.c | 6 ++--
> >> 5 files changed, 88 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 609e0df5d0..7bcdc4613b 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -2949,6 +2949,7 @@ static MigThrError migration_detect_error(MigrationState *s)
> >> {
> >> int ret;
> >> int state = s->state;
> >> + Error *local_error = NULL;
> >>
> >> if (state == MIGRATION_STATUS_CANCELLING ||
> >> state == MIGRATION_STATUS_CANCELLED) {
> >> @@ -2957,13 +2958,18 @@ static MigThrError migration_detect_error(MigrationState *s)
> >> }
> >>
> >> /* Try to detect any file errors */
> >> - ret = qemu_file_get_error(s->to_dst_file);
> >> -
> >> + ret = qemu_file_get_error_obj(s->to_dst_file, &local_error);
> >> if (!ret) {
> >> /* Everything is fine */
> >> + assert(!local_error);
> >> return MIG_THR_ERR_NONE;
> >> }
> >>
> >> + if (local_error) {
> >> + migrate_set_error(s, local_error);
> >> + error_free(local_error);
> >> + }
> >> +
> >> if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) {
> >> /*
> >> * For postcopy, we allow the network to be down for a
> >> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> >> index 8e639eb496..c382ea2d78 100644
> >> --- a/migration/qemu-file-channel.c
> >> +++ b/migration/qemu-file-channel.c
> >> @@ -33,7 +33,8 @@
> >> static ssize_t channel_writev_buffer(void *opaque,
> >> struct iovec *iov,
> >> int iovcnt,
> >> - int64_t pos)
> >> + int64_t pos,
> >> + Error **errp)
> >> {
> >> QIOChannel *ioc = QIO_CHANNEL(opaque);
> >> ssize_t done = 0;
> >> @@ -47,7 +48,7 @@ static ssize_t channel_writev_buffer(void *opaque,
> >>
> >> while (nlocal_iov > 0) {
> >> ssize_t len;
> >> - len = qio_channel_writev(ioc, local_iov, nlocal_iov, NULL);
> >> + len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
> >> if (len == QIO_CHANNEL_ERR_BLOCK) {
> >> if (qemu_in_coroutine()) {
> >> qio_channel_yield(ioc, G_IO_OUT);
> >> @@ -57,7 +58,6 @@ static ssize_t channel_writev_buffer(void *opaque,
> >> continue;
> >> }
> >> if (len < 0) {
> >> - /* XXX handle Error objects */
> >> done = -EIO;
> >> goto cleanup;
> >> }
> >> @@ -75,13 +75,14 @@ static ssize_t channel_writev_buffer(void *opaque,
> >> static ssize_t channel_get_buffer(void *opaque,
> >> uint8_t *buf,
> >> int64_t pos,
> >> - size_t size)
> >> + size_t size,
> >> + Error **errp)
> >> {
> >> QIOChannel *ioc = QIO_CHANNEL(opaque);
> >> ssize_t ret;
> >>
> >> do {
> >> - ret = qio_channel_read(ioc, (char *)buf, size, NULL);
> >> + ret = qio_channel_read(ioc, (char *)buf, size, errp);
> >> if (ret < 0) {
> >> if (ret == QIO_CHANNEL_ERR_BLOCK) {
> >> if (qemu_in_coroutine()) {
> >> @@ -90,7 +91,6 @@ static ssize_t channel_get_buffer(void *opaque,
> >> qio_channel_wait(ioc, G_IO_IN);
> >> }
> >> } else {
> >> - /* XXX handle Error * object */
> >> return -EIO;
> >> }
> >> }
> >> @@ -100,18 +100,20 @@ static ssize_t channel_get_buffer(void *opaque,
> >> }
> >>
> >> -static int channel_close(void *opaque)
> >> +static int channel_close(void *opaque, Error **errp)
> >> {
> >> + int ret;
> >> QIOChannel *ioc = QIO_CHANNEL(opaque);
> >> - qio_channel_close(ioc, NULL);
> >> + ret = qio_channel_close(ioc, errp);
> >> object_unref(OBJECT(ioc));
> >> - return 0;
> >> + return ret;
> >> }
> >>
> >> static int channel_shutdown(void *opaque,
> >> bool rd,
> >> - bool wr)
> >> + bool wr,
> >> + Error **errp)
> >> {
> >> QIOChannel *ioc = QIO_CHANNEL(opaque);
> >>
> >> @@ -125,8 +127,7 @@ static int channel_shutdown(void *opaque,
> >> } else {
> >> mode = QIO_CHANNEL_SHUTDOWN_WRITE;
> >> }
> >> - if (qio_channel_shutdown(ioc, mode, NULL) < 0) {
> >> - /* XXX handler Error * object */
> >> + if (qio_channel_shutdown(ioc, mode, errp) < 0) {
> >> return -EIO;
> >> }
> >> }
> >> @@ -135,11 +136,12 @@ static int channel_shutdown(void *opaque,
> >>
> >> static int channel_set_blocking(void *opaque,
> >> - bool enabled)
> >> + bool enabled,
> >> + Error **errp)
> >> {
> >> QIOChannel *ioc = QIO_CHANNEL(opaque);
> >>
> >> - if (qio_channel_set_blocking(ioc, enabled, NULL) < 0) {
> >> + if (qio_channel_set_blocking(ioc, enabled, errp) < 0) {
> >> return -1;
> >> }
> >> return 0;
> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> >> index 977b9ae07c..c52160e08b 100644
> >> --- a/migration/qemu-file.c
> >> +++ b/migration/qemu-file.c
> >> @@ -29,6 +29,7 @@
> >> #include "migration.h"
> >> #include "qemu-file.h"
> >> #include "trace.h"
> >> +#include "qapi/error.h"
> >>
> >> #define IO_BUF_SIZE 32768
> >> #define MAX_IOV_SIZE MIN(IOV_MAX, 64)
> >> @@ -52,6 +53,7 @@ struct QEMUFile {
> >> unsigned int iovcnt;
> >>
> >> int last_error;
> >> + Error *last_error_obj;
> >> };
> >>
> >> /*
> >> @@ -63,7 +65,7 @@ int qemu_file_shutdown(QEMUFile *f)
> >> if (!f->ops->shut_down) {
> >> return -ENOSYS;
> >> }
> >> - return f->ops->shut_down(f->opaque, true, true);
> >> + return f->ops->shut_down(f->opaque, true, true, NULL);
> >> }
> >>
> >> /*
> >> @@ -108,24 +110,55 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
> >> }
> >>
> >> /*
> >> - * Get last error for stream f
> >> + * Get last error for stream f with optional Error*
> >> *
> >> * Return negative error value if there has been an error on previous
> >> * operations, return 0 if no error happened.
> >> + * Optional, it returns Error* in errp, but it may be NULL even if return value
> >> + * is not 0.
> >> *
> >> */
> >> -int qemu_file_get_error(QEMUFile *f)
> >> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
> >> {
> >> + if (errp) {
> >> + *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
> >> + }
> >> return f->last_error;
> >> }
> >>
> >> -void qemu_file_set_error(QEMUFile *f, int ret)
> >> +/*
> >> + * Set the last error for stream f with optional Error*
> >> + */
> >> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err)
> >> {
> >> - if (f->last_error == 0) {
> >> + if (f->last_error == 0 && ret) {
> >> f->last_error = ret;
> >> + error_propagate(&f->last_error_obj, err);
> >> + } else if (err) {
> >> + error_report_err(err);
> >> }
> >> }
> >>
> >> +/*
> >> + * Get last error for stream f
> >> + *
> >> + * Return negative error value if there has been an error on previous
> >> + * operations, return 0 if no error happened.
> >> + *
> >> + */
> >> +int qemu_file_get_error(QEMUFile *f)
> >> +{
> >> + return qemu_file_get_error_obj(f, NULL);
> >> +}
> >> +
> >> +/*
> >> + * Set the last error for stream f
> >> + */
> >> +void qemu_file_set_error(QEMUFile *f, int ret)
> >> +{
> >> + qemu_file_set_error_obj(f, ret, NULL);
> >> +}
> >> +
> >> bool qemu_file_is_writable(QEMUFile *f)
> >> {
> >> return f->ops->writev_buffer;
> >> @@ -177,6 +210,7 @@ void qemu_fflush(QEMUFile *f)
> >> {
> >> ssize_t ret = 0;
> >> ssize_t expect = 0;
> >> + Error *local_error = NULL;
> >>
> >> if (!qemu_file_is_writable(f)) {
> >> return;
> >> @@ -184,7 +218,8 @@ void qemu_fflush(QEMUFile *f)
> >>
> >> if (f->iovcnt > 0) {
> >> expect = iov_size(f->iov, f->iovcnt);
> >> - ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos);
> >> + ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
> >> + &local_error);
> >>
> >> qemu_iovec_release_ram(f);
> >> }
> >> @@ -196,7 +231,7 @@ void qemu_fflush(QEMUFile *f)
> >> * data set we requested, so sanity check that.
> >> */
> >> if (ret != expect) {
> >> - qemu_file_set_error(f, ret < 0 ? ret : -EIO);
> >> + qemu_file_set_error_obj(f, ret < 0 ? ret : -EIO, local_error);
> >> }
> >> f->buf_index = 0;
> >> f->iovcnt = 0;
> >> @@ -284,6 +319,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> >> {
> >> int len;
> >> int pending;
> >> + Error *local_error = NULL;
> >>
> >> assert(!qemu_file_is_writable(f));
> >>
> >> @@ -295,14 +331,16 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> >> f->buf_size = pending;
> >>
> >> len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
> >> - IO_BUF_SIZE - pending);
> >> + IO_BUF_SIZE - pending, &local_error);
> >> if (len > 0) {
> >> f->buf_size += len;
> >> f->pos += len;
> >> } else if (len == 0) {
> >> - qemu_file_set_error(f, -EIO);
> >> + qemu_file_set_error_obj(f, -EIO, local_error);
> >> } else if (len != -EAGAIN) {
> >> - qemu_file_set_error(f, len);
> >> + qemu_file_set_error_obj(f, len, local_error);
> >> + } else {
> >> + error_free(local_error);
> >> }
> >>
> >> return len;
> >> @@ -328,7 +366,7 @@ int qemu_fclose(QEMUFile *f)
> >> ret = qemu_file_get_error(f);
> >>
> >> if (f->ops->close) {
> >> - int ret2 = f->ops->close(f->opaque);
> >> + int ret2 = f->ops->close(f->opaque, NULL);
> >> if (ret >= 0) {
> >> ret = ret2;
> >> }
> >> @@ -339,6 +377,7 @@ int qemu_fclose(QEMUFile *f)
> >> if (f->last_error) {
> >> ret = f->last_error;
> >> }
> >> + error_free(f->last_error_obj);
> >> g_free(f);
> >> trace_qemu_file_fclose();
> >> return ret;
> >> @@ -784,6 +823,6 @@ void qemu_put_counted_string(QEMUFile *f, const char *str)
> >> void qemu_file_set_blocking(QEMUFile *f, bool block)
> >> {
> >> if (f->ops->set_blocking) {
> >> - f->ops->set_blocking(f->opaque, block);
> >> + f->ops->set_blocking(f->opaque, block, NULL);
> >> }
> >> }
> >> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> >> index 13baf896bd..eb886db65f 100644
> >> --- a/migration/qemu-file.h
> >> +++ b/migration/qemu-file.h
> >> @@ -32,7 +32,8 @@
> >> * bytes actually read should be returned.
> >> */
> >> typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> >> - int64_t pos, size_t size);
> >> + int64_t pos, size_t size,
> >> + Error **errp);
> >>
> >> /* Close a file
> >> *
> >> @@ -41,7 +42,7 @@ typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> >> * The meaning of return value on success depends on the specific back-end being
> >> * used.
> >> */
> >> -typedef int (QEMUFileCloseFunc)(void *opaque);
> >> +typedef int (QEMUFileCloseFunc)(void *opaque, Error **errp);
> >>
> >> /* Called to return the OS file descriptor associated to the QEMUFile.
> >> */
> >> @@ -49,14 +50,15 @@ typedef int (QEMUFileGetFD)(void *opaque);
> >>
> >> /* Called to change the blocking mode of the file
> >> */
> >> -typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled);
> >> +typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled, Error **errp);
> >>
> >> /*
> >> * This function writes an iovec to file. The handler must write all
> >> * of the data or return a negative errno value.
> >> */
> >> typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
> >> - int iovcnt, int64_t pos);
> >> + int iovcnt, int64_t pos,
> >> + Error **errp);
> >>
> >> /*
> >> * This function provides hooks around different
> >> @@ -97,7 +99,8 @@ typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
> >> * Existing blocking reads/writes must be woken
> >> * Returns 0 on success, -err on error
> >> */
> >> -typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr);
> >> +typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr,
> >> + Error **errp);
> >>
> >> typedef struct QEMUFileOps {
> >> QEMUFileGetBufferFunc *get_buffer;
> >> @@ -149,6 +152,8 @@ void qemu_update_position(QEMUFile *f, size_t size);
> >> void qemu_file_reset_rate_limit(QEMUFile *f);
> >> void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
> >> int64_t qemu_file_get_rate_limit(QEMUFile *f);
> >> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
> >> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
> >> void qemu_file_set_error(QEMUFile *f, int ret);
> >> int qemu_file_shutdown(QEMUFile *f);
> >> QEMUFile *qemu_file_get_return_path(QEMUFile *f);
> >> diff --git a/migration/savevm.c b/migration/savevm.c
> >> index 34bcad3807..a619af744d 100644
> >> --- a/migration/savevm.c
> >> +++ b/migration/savevm.c
> >> @@ -124,7 +124,7 @@ static struct mig_cmd_args {
> >> /* savevm/loadvm support */
> >>
> >> static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> >> - int64_t pos)
> >> + int64_t pos, Error **errp)
> >> {
> >> int ret;
> >> QEMUIOVector qiov;
> >> @@ -139,12 +139,12 @@ static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> >> }
> >>
> >> static ssize_t block_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
> >> - size_t size)
> >> + size_t size, Error **errp)
> >> {
> >> return bdrv_load_vmstate(opaque, buf, pos, size);
> >> }
> >>
> >> -static int bdrv_fclose(void *opaque)
> >> +static int bdrv_fclose(void *opaque, Error **errp)
> >> {
> >> return bdrv_flush(opaque);
> >> }
> >> --
> >> 2.21.0
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
> Regards,
> Yury
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> Currently, there is no information about error if outgoing migration was failed
> because of file channel errors.
> Example (QMP session):
> -> { "execute": "migrate", "arguments": { "uri": "exec:head -c 1" }}
> <- { "return": {} }
> ...
> -> { "execute": "query-migrate" }
> <- { "return": { "status": "failed" }} // There is not error's description
>
> And even in the QEMU's output there is nothing.
>
> This patch
> 1) Adds errp for the most of QEMUFileOps
> 2) Adds qemu_file_get_error_obj/qemu_file_set_error_obj
> 3) And finally using of qemu_file_get_error_obj in migration.c
>
> And now, the status for the mentioned fail will be:
> -> { "execute": "query-migrate" }
> <- { "return": { "status": "failed",
> "error-desc": "Unable to write to command: Broken pipe" }}
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
Quued for 4.2
> ---
> migration/migration.c | 10 ++++--
> migration/qemu-file-channel.c | 30 +++++++++--------
> migration/qemu-file.c | 63 ++++++++++++++++++++++++++++-------
> migration/qemu-file.h | 15 ++++++---
> migration/savevm.c | 6 ++--
> 5 files changed, 88 insertions(+), 36 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 609e0df5d0..7bcdc4613b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2949,6 +2949,7 @@ static MigThrError migration_detect_error(MigrationState *s)
> {
> int ret;
> int state = s->state;
> + Error *local_error = NULL;
>
> if (state == MIGRATION_STATUS_CANCELLING ||
> state == MIGRATION_STATUS_CANCELLED) {
> @@ -2957,13 +2958,18 @@ static MigThrError migration_detect_error(MigrationState *s)
> }
>
> /* Try to detect any file errors */
> - ret = qemu_file_get_error(s->to_dst_file);
> -
> + ret = qemu_file_get_error_obj(s->to_dst_file, &local_error);
> if (!ret) {
> /* Everything is fine */
> + assert(!local_error);
> return MIG_THR_ERR_NONE;
> }
>
> + if (local_error) {
> + migrate_set_error(s, local_error);
> + error_free(local_error);
> + }
> +
> if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) {
> /*
> * For postcopy, we allow the network to be down for a
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 8e639eb496..c382ea2d78 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -33,7 +33,8 @@
> static ssize_t channel_writev_buffer(void *opaque,
> struct iovec *iov,
> int iovcnt,
> - int64_t pos)
> + int64_t pos,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ssize_t done = 0;
> @@ -47,7 +48,7 @@ static ssize_t channel_writev_buffer(void *opaque,
>
> while (nlocal_iov > 0) {
> ssize_t len;
> - len = qio_channel_writev(ioc, local_iov, nlocal_iov, NULL);
> + len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
> if (len == QIO_CHANNEL_ERR_BLOCK) {
> if (qemu_in_coroutine()) {
> qio_channel_yield(ioc, G_IO_OUT);
> @@ -57,7 +58,6 @@ static ssize_t channel_writev_buffer(void *opaque,
> continue;
> }
> if (len < 0) {
> - /* XXX handle Error objects */
> done = -EIO;
> goto cleanup;
> }
> @@ -75,13 +75,14 @@ static ssize_t channel_writev_buffer(void *opaque,
> static ssize_t channel_get_buffer(void *opaque,
> uint8_t *buf,
> int64_t pos,
> - size_t size)
> + size_t size,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ssize_t ret;
>
> do {
> - ret = qio_channel_read(ioc, (char *)buf, size, NULL);
> + ret = qio_channel_read(ioc, (char *)buf, size, errp);
> if (ret < 0) {
> if (ret == QIO_CHANNEL_ERR_BLOCK) {
> if (qemu_in_coroutine()) {
> @@ -90,7 +91,6 @@ static ssize_t channel_get_buffer(void *opaque,
> qio_channel_wait(ioc, G_IO_IN);
> }
> } else {
> - /* XXX handle Error * object */
> return -EIO;
> }
> }
> @@ -100,18 +100,20 @@ static ssize_t channel_get_buffer(void *opaque,
> }
>
>
> -static int channel_close(void *opaque)
> +static int channel_close(void *opaque, Error **errp)
> {
> + int ret;
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> - qio_channel_close(ioc, NULL);
> + ret = qio_channel_close(ioc, errp);
> object_unref(OBJECT(ioc));
> - return 0;
> + return ret;
> }
>
>
> static int channel_shutdown(void *opaque,
> bool rd,
> - bool wr)
> + bool wr,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
>
> @@ -125,8 +127,7 @@ static int channel_shutdown(void *opaque,
> } else {
> mode = QIO_CHANNEL_SHUTDOWN_WRITE;
> }
> - if (qio_channel_shutdown(ioc, mode, NULL) < 0) {
> - /* XXX handler Error * object */
> + if (qio_channel_shutdown(ioc, mode, errp) < 0) {
> return -EIO;
> }
> }
> @@ -135,11 +136,12 @@ static int channel_shutdown(void *opaque,
>
>
> static int channel_set_blocking(void *opaque,
> - bool enabled)
> + bool enabled,
> + Error **errp)
> {
> QIOChannel *ioc = QIO_CHANNEL(opaque);
>
> - if (qio_channel_set_blocking(ioc, enabled, NULL) < 0) {
> + if (qio_channel_set_blocking(ioc, enabled, errp) < 0) {
> return -1;
> }
> return 0;
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 977b9ae07c..c52160e08b 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -29,6 +29,7 @@
> #include "migration.h"
> #include "qemu-file.h"
> #include "trace.h"
> +#include "qapi/error.h"
>
> #define IO_BUF_SIZE 32768
> #define MAX_IOV_SIZE MIN(IOV_MAX, 64)
> @@ -52,6 +53,7 @@ struct QEMUFile {
> unsigned int iovcnt;
>
> int last_error;
> + Error *last_error_obj;
> };
>
> /*
> @@ -63,7 +65,7 @@ int qemu_file_shutdown(QEMUFile *f)
> if (!f->ops->shut_down) {
> return -ENOSYS;
> }
> - return f->ops->shut_down(f->opaque, true, true);
> + return f->ops->shut_down(f->opaque, true, true, NULL);
> }
>
> /*
> @@ -108,24 +110,55 @@ void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks)
> }
>
> /*
> - * Get last error for stream f
> + * Get last error for stream f with optional Error*
> *
> * Return negative error value if there has been an error on previous
> * operations, return 0 if no error happened.
> + * Optional, it returns Error* in errp, but it may be NULL even if return value
> + * is not 0.
> *
> */
> -int qemu_file_get_error(QEMUFile *f)
> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
> {
> + if (errp) {
> + *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
> + }
> return f->last_error;
> }
>
> -void qemu_file_set_error(QEMUFile *f, int ret)
> +/*
> + * Set the last error for stream f with optional Error*
> + */
> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err)
> {
> - if (f->last_error == 0) {
> + if (f->last_error == 0 && ret) {
> f->last_error = ret;
> + error_propagate(&f->last_error_obj, err);
> + } else if (err) {
> + error_report_err(err);
> }
> }
>
> +/*
> + * Get last error for stream f
> + *
> + * Return negative error value if there has been an error on previous
> + * operations, return 0 if no error happened.
> + *
> + */
> +int qemu_file_get_error(QEMUFile *f)
> +{
> + return qemu_file_get_error_obj(f, NULL);
> +}
> +
> +/*
> + * Set the last error for stream f
> + */
> +void qemu_file_set_error(QEMUFile *f, int ret)
> +{
> + qemu_file_set_error_obj(f, ret, NULL);
> +}
> +
> bool qemu_file_is_writable(QEMUFile *f)
> {
> return f->ops->writev_buffer;
> @@ -177,6 +210,7 @@ void qemu_fflush(QEMUFile *f)
> {
> ssize_t ret = 0;
> ssize_t expect = 0;
> + Error *local_error = NULL;
>
> if (!qemu_file_is_writable(f)) {
> return;
> @@ -184,7 +218,8 @@ void qemu_fflush(QEMUFile *f)
>
> if (f->iovcnt > 0) {
> expect = iov_size(f->iov, f->iovcnt);
> - ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos);
> + ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt, f->pos,
> + &local_error);
>
> qemu_iovec_release_ram(f);
> }
> @@ -196,7 +231,7 @@ void qemu_fflush(QEMUFile *f)
> * data set we requested, so sanity check that.
> */
> if (ret != expect) {
> - qemu_file_set_error(f, ret < 0 ? ret : -EIO);
> + qemu_file_set_error_obj(f, ret < 0 ? ret : -EIO, local_error);
> }
> f->buf_index = 0;
> f->iovcnt = 0;
> @@ -284,6 +319,7 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> {
> int len;
> int pending;
> + Error *local_error = NULL;
>
> assert(!qemu_file_is_writable(f));
>
> @@ -295,14 +331,16 @@ static ssize_t qemu_fill_buffer(QEMUFile *f)
> f->buf_size = pending;
>
> len = f->ops->get_buffer(f->opaque, f->buf + pending, f->pos,
> - IO_BUF_SIZE - pending);
> + IO_BUF_SIZE - pending, &local_error);
> if (len > 0) {
> f->buf_size += len;
> f->pos += len;
> } else if (len == 0) {
> - qemu_file_set_error(f, -EIO);
> + qemu_file_set_error_obj(f, -EIO, local_error);
> } else if (len != -EAGAIN) {
> - qemu_file_set_error(f, len);
> + qemu_file_set_error_obj(f, len, local_error);
> + } else {
> + error_free(local_error);
> }
>
> return len;
> @@ -328,7 +366,7 @@ int qemu_fclose(QEMUFile *f)
> ret = qemu_file_get_error(f);
>
> if (f->ops->close) {
> - int ret2 = f->ops->close(f->opaque);
> + int ret2 = f->ops->close(f->opaque, NULL);
> if (ret >= 0) {
> ret = ret2;
> }
> @@ -339,6 +377,7 @@ int qemu_fclose(QEMUFile *f)
> if (f->last_error) {
> ret = f->last_error;
> }
> + error_free(f->last_error_obj);
> g_free(f);
> trace_qemu_file_fclose();
> return ret;
> @@ -784,6 +823,6 @@ void qemu_put_counted_string(QEMUFile *f, const char *str)
> void qemu_file_set_blocking(QEMUFile *f, bool block)
> {
> if (f->ops->set_blocking) {
> - f->ops->set_blocking(f->opaque, block);
> + f->ops->set_blocking(f->opaque, block, NULL);
> }
> }
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 13baf896bd..eb886db65f 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -32,7 +32,8 @@
> * bytes actually read should be returned.
> */
> typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> - int64_t pos, size_t size);
> + int64_t pos, size_t size,
> + Error **errp);
>
> /* Close a file
> *
> @@ -41,7 +42,7 @@ typedef ssize_t (QEMUFileGetBufferFunc)(void *opaque, uint8_t *buf,
> * The meaning of return value on success depends on the specific back-end being
> * used.
> */
> -typedef int (QEMUFileCloseFunc)(void *opaque);
> +typedef int (QEMUFileCloseFunc)(void *opaque, Error **errp);
>
> /* Called to return the OS file descriptor associated to the QEMUFile.
> */
> @@ -49,14 +50,15 @@ typedef int (QEMUFileGetFD)(void *opaque);
>
> /* Called to change the blocking mode of the file
> */
> -typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled);
> +typedef int (QEMUFileSetBlocking)(void *opaque, bool enabled, Error **errp);
>
> /*
> * This function writes an iovec to file. The handler must write all
> * of the data or return a negative errno value.
> */
> typedef ssize_t (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
> - int iovcnt, int64_t pos);
> + int iovcnt, int64_t pos,
> + Error **errp);
>
> /*
> * This function provides hooks around different
> @@ -97,7 +99,8 @@ typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
> * Existing blocking reads/writes must be woken
> * Returns 0 on success, -err on error
> */
> -typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr);
> +typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr,
> + Error **errp);
>
> typedef struct QEMUFileOps {
> QEMUFileGetBufferFunc *get_buffer;
> @@ -149,6 +152,8 @@ void qemu_update_position(QEMUFile *f, size_t size);
> void qemu_file_reset_rate_limit(QEMUFile *f);
> void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
> int64_t qemu_file_get_rate_limit(QEMUFile *f);
> +int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
> +void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
> void qemu_file_set_error(QEMUFile *f, int ret);
> int qemu_file_shutdown(QEMUFile *f);
> QEMUFile *qemu_file_get_return_path(QEMUFile *f);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 34bcad3807..a619af744d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -124,7 +124,7 @@ static struct mig_cmd_args {
> /* savevm/loadvm support */
>
> static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> - int64_t pos)
> + int64_t pos, Error **errp)
> {
> int ret;
> QEMUIOVector qiov;
> @@ -139,12 +139,12 @@ static ssize_t block_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
> }
>
> static ssize_t block_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
> - size_t size)
> + size_t size, Error **errp)
> {
> return bdrv_load_vmstate(opaque, buf, pos, size);
> }
>
> -static int bdrv_fclose(void *opaque)
> +static int bdrv_fclose(void *opaque, Error **errp)
> {
> return bdrv_flush(opaque);
> }
> --
> 2.21.0
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
© 2016 - 2024 Red Hat, Inc.