From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Yury Kotov <yury-kotov@yandex-team.ru>
Cc: "open list:All patches CC here" <qemu-devel@nongnu.org>,
yc-core@yandex-team.ru, Juan Quintela <quintela@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] migration: Add error_desc for file channel errors
Date: Wed, 7 Aug 2019 18:08:54 +0100 [thread overview]
Message-ID: <20190807170854.GA27871@work-vm> (raw)
In-Reply-To: <20190422103420.15686-1-yury-kotov@yandex-team.ru>
* 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
prev parent reply other threads:[~2019-08-07 17:09 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-22 10:34 [Qemu-devel] [PATCH] migration: Add error_desc for file channel errors Yury Kotov
2019-06-07 9:02 ` Yury Kotov
2019-06-14 16:39 ` Dr. David Alan Gilbert
2019-07-17 10:19 ` Yury Kotov
2019-08-06 15:51 ` Dr. David Alan Gilbert
2019-08-07 17:08 ` Dr. David Alan Gilbert [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190807170854.GA27871@work-vm \
--to=dgilbert@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
--cc=yc-core@yandex-team.ru \
--cc=yury-kotov@yandex-team.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.