* [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
@ 2019-09-17 10:20 ` Greg Kurz
2019-09-17 11:21 ` Cornelia Huck
2019-09-17 14:36 ` Eric Blake
2019-09-17 10:20 ` [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint() Greg Kurz
` (16 subsequent siblings)
17 siblings, 2 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
error_setg() and error_propagate(), as well as their variants, cause
QEMU to terminate when called with &error_fatal or &error_abort. This
prevents to add hints since error_append_hint() isn't even called in
this case.
It means that error_append_hint() should only be used with a local
error object, and then propagate this local error to the caller.
Document this in <qapi/error.h> .
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/qapi/error.h | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3f95141a01a8..28174329ba71 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -27,6 +27,13 @@
* error_setg(&err, "invalid quark\n"
* "Valid quarks are up, down, strange, charm, top, bottom.");
*
+ * Dot *not* pass the errp parameter from the caller
+ * error_setg(errp, "invalid quark");
+ * error_append_hint(errp, "Valid quarks are up, down, strange, "
+ * "charm, top, bottom.\n");
+ * because error_setg() terminates QEMU if @errp is &error_fatal or
+ * &error_abort, and the hints are not even added.
+ *
* Report an error to the current monitor if we have one, else stderr:
* error_report_err(err);
* This frees the error object.
@@ -252,7 +259,9 @@ void error_prepend(Error **errp, const char *fmt, ...)
* error message.
* @errp may be NULL, but not &error_fatal or &error_abort.
* Trivially the case if you call it only after error_setg() or
- * error_propagate().
+ * error_propagate(). CAUTION: error_setg() and error_propagate() do
+ * not return when passed &error_fatal. Always use a local error object
+ * and propagate it to the caller.
* May be called multiple times. The resulting hint should end with a
* newline.
*/
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation
2019-09-17 10:20 ` [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation Greg Kurz
@ 2019-09-17 11:21 ` Cornelia Huck
2019-09-17 14:36 ` Eric Blake
1 sibling, 0 replies; 49+ messages in thread
From: Cornelia Huck @ 2019-09-17 11:21 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 12:20:37 +0200
Greg Kurz <groug@kaod.org> wrote:
> error_setg() and error_propagate(), as well as their variants, cause
> QEMU to terminate when called with &error_fatal or &error_abort. This
> prevents to add hints since error_append_hint() isn't even called in
> this case.
>
> It means that error_append_hint() should only be used with a local
> error object, and then propagate this local error to the caller.
>
> Document this in <qapi/error.h> .
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> include/qapi/error.h | 11 ++++++++++-
It's a bit awkward requiring a local object, but better safe than sorry.
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation
2019-09-17 10:20 ` [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation Greg Kurz
2019-09-17 11:21 ` Cornelia Huck
@ 2019-09-17 14:36 ` Eric Blake
1 sibling, 0 replies; 49+ messages in thread
From: Eric Blake @ 2019-09-17 14:36 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
qemu-block, Juan Quintela, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, qemu-arm, John Snow, Richard Henderson,
Kevin Wolf, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On 9/17/19 5:20 AM, Greg Kurz wrote:
> error_setg() and error_propagate(), as well as their variants, cause
> QEMU to terminate when called with &error_fatal or &error_abort. This
> prevents to add hints since error_append_hint() isn't even called in
s/to add/adding/
> this case.
>
> It means that error_append_hint() should only be used with a local
> error object, and then propagate this local error to the caller.
>
> Document this in <qapi/error.h> .
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> include/qapi/error.h | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
2019-09-17 10:20 ` [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation Greg Kurz
@ 2019-09-17 10:20 ` Greg Kurz
2019-09-17 13:25 ` Vladimir Sementsov-Ogievskiy
2019-09-17 14:39 ` Eric Blake
2019-09-17 10:20 ` [Qemu-devel] [PATCH 03/17] char/spice: " Greg Kurz
` (15 subsequent siblings)
17 siblings, 2 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
block/backup.c | 7 +++++--
block/dirty-bitmap.c | 7 +++++--
block/file-posix.c | 20 +++++++++++++-------
block/gluster.c | 23 +++++++++++++++--------
block/qcow.c | 10 ++++++----
block/qcow2.c | 7 +++++--
block/vhdx-log.c | 7 +++++--
block/vpc.c | 7 +++++--
8 files changed, 59 insertions(+), 29 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6db..d8c422a0e3bc 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
BACKUP_CLUSTER_SIZE_DEFAULT);
return BACKUP_CLUSTER_SIZE_DEFAULT;
} else if (ret < 0 && !target->backing) {
- error_setg_errno(errp, -ret,
+ Error *local_err = NULL;
+
+ error_setg_errno(&local_err, -ret,
"Couldn't determine the cluster size of the target image, "
"which has no backing file");
- error_append_hint(errp,
+ error_append_hint(&local_err,
"Aborting, since this may create an unusable destination image\n");
+ error_propagate(errp, local_err);
return ret;
} else if (ret < 0 && target->backing) {
/* Not fatal; just trudge on ahead. */
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c8f..b31017a77d51 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -251,10 +251,13 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
if ((flags & BDRV_BITMAP_INCONSISTENT) &&
bdrv_dirty_bitmap_inconsistent(bitmap)) {
- error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Bitmap '%s' is inconsistent and cannot be used",
bitmap->name);
- error_append_hint(errp, "Try block-dirty-bitmap-remove to delete"
+ error_append_hint(&local_err, "Try block-dirty-bitmap-remove to delete"
" this bitmap from disk");
+ error_propagate(errp, local_err);
return -1;
}
diff --git a/block/file-posix.c b/block/file-posix.c
index f12c06de2df5..9a95f7e94123 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -389,8 +389,11 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
}
if (!s->buf_align || !bs->bl.request_alignment) {
- error_setg(errp, "Could not find working O_DIRECT alignment");
- error_append_hint(errp, "Try cache.direct=off\n");
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Could not find working O_DIRECT alignment");
+ error_append_hint(&local_err, "Try cache.direct=off\n");
+ error_propagate(errp, local_err);
}
}
@@ -845,16 +848,18 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
}
ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
~s->shared_perm | ~new_shared,
- false, errp);
+ false, &local_err);
if (!ret) {
- ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, errp);
+ ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, &local_err);
if (!ret) {
return 0;
}
- error_append_hint(errp,
+ error_append_hint(&local_err,
"Is another process using the image [%s]?\n",
bs->filename);
}
+ error_propagate(errp, local_err);
+ local_err = NULL; /* for fall through */
op = RAW_PL_ABORT;
/* fall through to unlock bytes. */
case RAW_PL_ABORT:
@@ -2277,11 +2282,12 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
}
/* Step two: Check that nobody else has taken conflicting locks */
- result = raw_check_lock_bytes(fd, perm, shared, errp);
+ result = raw_check_lock_bytes(fd, perm, shared, &local_err);
if (result < 0) {
- error_append_hint(errp,
+ error_append_hint(&local_err,
"Is another process using the image [%s]?\n",
file_opts->filename);
+ error_propagate(errp, local_err);
goto out_unlock;
}
diff --git a/block/gluster.c b/block/gluster.c
index 64028b2cba30..dce42884f97c 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -473,26 +473,29 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
ret = glfs_init(glfs);
if (ret) {
- error_setg(errp, "Gluster connection for volume %s, path %s failed"
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Gluster connection for volume %s, path %s failed"
" to connect", gconf->volume, gconf->path);
for (server = gconf->server; server; server = server->next) {
if (server->value->type == SOCKET_ADDRESS_TYPE_UNIX) {
- error_append_hint(errp, "hint: failed on socket %s ",
+ error_append_hint(&local_err, "hint: failed on socket %s ",
server->value->u.q_unix.path);
} else {
- error_append_hint(errp, "hint: failed on host %s and port %s ",
+ error_append_hint(&local_err, "hint: failed on host %s and port %s ",
server->value->u.inet.host,
server->value->u.inet.port);
}
}
- error_append_hint(errp, "Please refer to gluster logs for more info\n");
+ error_append_hint(&local_err, "Please refer to gluster logs for more info\n");
/* glfs_init sometimes doesn't set errno although docs suggest that */
if (errno == 0) {
errno = EINVAL;
}
+ error_propagate(errp, local_err);
goto out;
}
return glfs;
@@ -695,20 +698,23 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
QDict *options, Error **errp)
{
int ret;
+ Error *local_err = NULL;
+
if (filename) {
ret = qemu_gluster_parse_uri(gconf, filename);
if (ret < 0) {
- error_setg(errp, "invalid URI %s", filename);
- error_append_hint(errp, "Usage: file=gluster[+transport]://"
+ error_setg(&local_err, "invalid URI %s", filename);
+ error_append_hint(&local_err, "Usage: file=gluster[+transport]://"
"[host[:port]]volume/path[?socket=...]"
"[,file.debug=N]"
"[,file.logfile=/path/filename.log]\n");
+ error_propagate(errp, local_err);
return ret;
}
} else {
- ret = qemu_gluster_parse_json(gconf, options, errp);
+ ret = qemu_gluster_parse_json(gconf, options, &local_err);
if (ret < 0) {
- error_append_hint(errp, "Usage: "
+ error_append_hint(&local_err, "Usage: "
"-drive driver=qcow2,file.driver=gluster,"
"file.volume=testvol,file.path=/path/a.qcow2"
"[,file.debug=9]"
@@ -719,6 +725,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
"file.server.1.transport=unix,"
"file.server.1.path=/var/run/glusterd.socket ..."
"\n");
+ error_propagate(errp, local_err);
return ret;
}
}
diff --git a/block/qcow.c b/block/qcow.c
index 5bdf72ba33ce..9049573b52b2 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -156,11 +156,12 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
if (header.version != QCOW_VERSION) {
- error_setg(errp, "qcow (v%d) does not support qcow version %" PRIu32,
+ error_setg(&local_err, "qcow (v%d) does not support qcow version %" PRIu32,
QCOW_VERSION, header.version);
if (header.version == 2 || header.version == 3) {
- error_append_hint(errp, "Try the 'qcow2' driver instead.\n");
+ error_append_hint(&local_err, "Try the 'qcow2' driver instead.\n");
}
+ error_propagate(errp, local_err);
ret = -ENOTSUP;
goto fail;
@@ -189,14 +190,15 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
if (s->crypt_method_header) {
if (bdrv_uses_whitelist() &&
s->crypt_method_header == QCOW_CRYPT_AES) {
- error_setg(errp,
+ error_setg(&local_err,
"Use of AES-CBC encrypted qcow images is no longer "
"supported in system emulators");
- error_append_hint(errp,
+ error_append_hint(&local_err,
"You can use 'qemu-img convert' to convert your "
"image to an alternative supported format, such "
"as unencrypted qcow, or raw with the LUKS "
"format instead.\n");
+ error_propagate(errp, local_err);
ret = -ENOSYS;
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 57734f20cf8d..2c52139b7385 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1357,14 +1357,17 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
if (s->crypt_method_header) {
if (bdrv_uses_whitelist() &&
s->crypt_method_header == QCOW_CRYPT_AES) {
- error_setg(errp,
+ Error *local_err = NULL;
+
+ error_setg(&local_err,
"Use of AES-CBC encrypted qcow2 images is no longer "
"supported in system emulators");
- error_append_hint(errp,
+ error_append_hint(&local_err,
"You can use 'qemu-img convert' to convert your "
"image to an alternative supported format, such "
"as unencrypted qcow2, or raw with the LUKS "
"format instead.\n");
+ error_propagate(errp, local_err);
ret = -ENOSYS;
goto fail;
}
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index fdd3a7adc378..9f4de9cbcdb6 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -802,15 +802,18 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
if (logs.valid) {
if (bs->read_only) {
+ Error *local_err = NULL;
+
bdrv_refresh_filename(bs);
ret = -EPERM;
- error_setg(errp,
+ error_setg(&local_err,
"VHDX image file '%s' opened read-only, but "
"contains a log that needs to be replayed",
bs->filename);
- error_append_hint(errp, "To replay the log, run:\n"
+ error_append_hint(&local_err, "To replay the log, run:\n"
"qemu-img check -r all '%s'\n",
bs->filename);
+ error_propagate(errp, local_err);
goto exit;
}
/* now flush the log */
diff --git a/block/vpc.c b/block/vpc.c
index 5cd38907808b..facd7cdb2c98 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1028,12 +1028,15 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
}
if (total_size != total_sectors * BDRV_SECTOR_SIZE) {
- error_setg(errp, "The requested image size cannot be represented in "
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "The requested image size cannot be represented in "
"CHS geometry");
- error_append_hint(errp, "Try size=%llu or force-size=on (the "
+ error_append_hint(&local_err, "Try size=%llu or force-size=on (the "
"latter makes the image incompatible with "
"Virtual PC)",
total_sectors * BDRV_SECTOR_SIZE);
+ error_propagate(errp, local_err);
ret = -EINVAL;
goto out;
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 ` [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint() Greg Kurz
@ 2019-09-17 13:25 ` Vladimir Sementsov-Ogievskiy
2019-09-17 15:37 ` Greg Kurz
2019-09-17 14:39 ` Eric Blake
1 sibling, 1 reply; 49+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-17 13:25 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Max Reitz,
qemu-block, Juan Quintela, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, qemu-s390x@nongnu.org",
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, qemu-arm, John Snow, Richard Henderson,
Kevin Wolf, berrange, Cornelia Huck, Subbaraya Sundeep, qemu-ppc,
Paolo Bonzini
17.09.2019 13:20, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> block/backup.c | 7 +++++--
> block/dirty-bitmap.c | 7 +++++--
> block/file-posix.c | 20 +++++++++++++-------
> block/gluster.c | 23 +++++++++++++++--------
> block/qcow.c | 10 ++++++----
> block/qcow2.c | 7 +++++--
> block/vhdx-log.c | 7 +++++--
> block/vpc.c | 7 +++++--
> 8 files changed, 59 insertions(+), 29 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6db..d8c422a0e3bc 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> BACKUP_CLUSTER_SIZE_DEFAULT);
> return BACKUP_CLUSTER_SIZE_DEFAULT;
> } else if (ret < 0 && !target->backing) {
> - error_setg_errno(errp, -ret,
> + Error *local_err = NULL;
> +
> + error_setg_errno(&local_err, -ret,
> "Couldn't determine the cluster size of the target image, "
> "which has no backing file");
> - error_append_hint(errp,
> + error_append_hint(&local_err,
> "Aborting, since this may create an unusable destination image\n");
> + error_propagate(errp, local_err);
> return ret;
> } else if (ret < 0 && target->backing) {
> /* Not fatal; just trudge on ahead. */
Pain.. Do we need these hints, if they are so painful?
At least for cases like this, we can create helper function
error_setg_errno_hint(..., error, hint)
But what could be done when we call function, which may or may not set errp?
ret = f(errp);
if (ret) {
error_append_hint(errp, hint);
}
Hmmm..
Can it look like
ret = f(..., hint_helper(errp, hint))
?
I can't imagine how to do it, as someone should remove hint from error_abort object on
success path..
But seems, the following is possible, which seems better for me than local-error approach:
error_push_hint(errp, hint);
ret = f(.., errp);
error_pop_hint(errp);
===
Continue thinking on this:
It may look like just
ret = f(..., set_hint(errp, hint));
or (just to split long line):
set_hint(errp, hint);
ret = f(..., errp);
if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
which may be just one call at function start of macro, which will call error_push_hint(errp) and
define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
exit..
Or, may be, more direct way to set cleanup for function exists?
===
Also, we can implement some code generation, to generate for functions with errp argument
wrappers with additional hint parameter, and just use these wrappers..
===
If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
real issue and much effort has already been spent. May be one day I'll try to rewrite it...
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 13:25 ` Vladimir Sementsov-Ogievskiy
@ 2019-09-17 15:37 ` Greg Kurz
2019-09-17 17:40 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 15:37 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Max Reitz, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, Richard Henderson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, David Gibson, Kevin Wolf, berrange, Cornelia Huck,
qemu-s390x, Subbaraya Sundeep, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 13:25:03 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 17.09.2019 13:20, Greg Kurz wrote:
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > block/backup.c | 7 +++++--
> > block/dirty-bitmap.c | 7 +++++--
> > block/file-posix.c | 20 +++++++++++++-------
> > block/gluster.c | 23 +++++++++++++++--------
> > block/qcow.c | 10 ++++++----
> > block/qcow2.c | 7 +++++--
> > block/vhdx-log.c | 7 +++++--
> > block/vpc.c | 7 +++++--
> > 8 files changed, 59 insertions(+), 29 deletions(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index 763f0d7ff6db..d8c422a0e3bc 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> > BACKUP_CLUSTER_SIZE_DEFAULT);
> > return BACKUP_CLUSTER_SIZE_DEFAULT;
> > } else if (ret < 0 && !target->backing) {
> > - error_setg_errno(errp, -ret,
> > + Error *local_err = NULL;
> > +
> > + error_setg_errno(&local_err, -ret,
> > "Couldn't determine the cluster size of the target image, "
> > "which has no backing file");
> > - error_append_hint(errp,
> > + error_append_hint(&local_err,
> > "Aborting, since this may create an unusable destination image\n");
> > + error_propagate(errp, local_err);
> > return ret;
> > } else if (ret < 0 && target->backing) {
> > /* Not fatal; just trudge on ahead. */
>
>
> Pain.. Do we need these hints, if they are so painful?
>
I agree that the one above doesn't qualify as a useful hint.
It just tells that QEMU is giving up and gives no indication
to the user on how to avoid the issue. It should probably be
dropped.
> At least for cases like this, we can create helper function
>
> error_setg_errno_hint(..., error, hint)
Not very useful if hint has to be forged separately with
g_sprintf(), and we can't have such a thing as:
error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
>
> But what could be done when we call function, which may or may not set errp?
>
> ret = f(errp);
> if (ret) {
> error_append_hint(errp, hint);
> }
>
Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
ends up calling exit().
> Hmmm..
>
> Can it look like
>
> ret = f(..., hint_helper(errp, hint))
>
> ?
>
Nope, hint_helper() will get called before f() and things are worse.
If errp is NULL then error_append_hint() does nothing and if it is
&error_fatal then it aborts.
> I can't imagine how to do it, as someone should remove hint from error_abort object on
> success path..
>
> But seems, the following is possible, which seems better for me than local-error approach:
>
> error_push_hint(errp, hint);
> ret = f(.., errp);
> error_pop_hint(errp);
>
Matter of taste... also, it looks awkward to come up with a hint
before knowing what happened. I mean the appropriate hint could
depend on the value returned by f() and/or errno for example.
> ===
>
> Continue thinking on this:
>
> It may look like just
> ret = f(..., set_hint(errp, hint));
>
> or (just to split long line):
> set_hint(errp, hint);
> ret = f(..., errp);
>
> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
> which may be just one call at function start of macro, which will call error_push_hint(errp) and
> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
> exit..
>
> Or, may be, more direct way to set cleanup for function exists?
>
> ===
>
> Also, we can implement some code generation, to generate for functions with errp argument
> wrappers with additional hint parameter, and just use these wrappers..
>
> ===
>
> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
>
For the reason exposed above, I don't think it makes sense to
build the hint before calling a function that calls error_setg().
I'm afraid we're stuck with local_err... it is then up to the
people to make it as less painful as possible.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 15:37 ` Greg Kurz
@ 2019-09-17 17:40 ` Vladimir Sementsov-Ogievskiy
2019-09-18 7:58 ` Greg Kurz
0 siblings, 1 reply; 49+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-17 17:40 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Max Reitz, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, Richard Henderson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, David Gibson, Kevin Wolf, berrange, Cornelia Huck,
qemu-s390x, Subbaraya Sundeep, qemu-ppc, Paolo Bonzini
17.09.2019 18:37, Greg Kurz wrote:
> On Tue, 17 Sep 2019 13:25:03 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> 17.09.2019 13:20, Greg Kurz wrote:
>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>> block/backup.c | 7 +++++--
>>> block/dirty-bitmap.c | 7 +++++--
>>> block/file-posix.c | 20 +++++++++++++-------
>>> block/gluster.c | 23 +++++++++++++++--------
>>> block/qcow.c | 10 ++++++----
>>> block/qcow2.c | 7 +++++--
>>> block/vhdx-log.c | 7 +++++--
>>> block/vpc.c | 7 +++++--
>>> 8 files changed, 59 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>> BACKUP_CLUSTER_SIZE_DEFAULT);
>>> return BACKUP_CLUSTER_SIZE_DEFAULT;
>>> } else if (ret < 0 && !target->backing) {
>>> - error_setg_errno(errp, -ret,
>>> + Error *local_err = NULL;
>>> +
>>> + error_setg_errno(&local_err, -ret,
>>> "Couldn't determine the cluster size of the target image, "
>>> "which has no backing file");
>>> - error_append_hint(errp,
>>> + error_append_hint(&local_err,
>>> "Aborting, since this may create an unusable destination image\n");
>>> + error_propagate(errp, local_err);
>>> return ret;
>>> } else if (ret < 0 && target->backing) {
>>> /* Not fatal; just trudge on ahead. */
>>
>>
>> Pain.. Do we need these hints, if they are so painful?
>>
>
> I agree that the one above doesn't qualify as a useful hint.
> It just tells that QEMU is giving up and gives no indication
> to the user on how to avoid the issue. It should probably be
> dropped.
>
>> At least for cases like this, we can create helper function
>>
>> error_setg_errno_hint(..., error, hint)
>
> Not very useful if hint has to be forged separately with
> g_sprintf(), and we can't have such a thing as:
>
> error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
>
>>
>> But what could be done when we call function, which may or may not set errp?
>>
>> ret = f(errp);
>> if (ret) {
>> error_append_hint(errp, hint);
>> }
>>
>
> Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
> ends up calling exit().
>
>> Hmmm..
>>
>> Can it look like
>>
>> ret = f(..., hint_helper(errp, hint))
>>
>> ?
>>
>
> Nope, hint_helper() will get called before f() and things are worse.
> If errp is NULL then error_append_hint() does nothing and if it is
> &error_fatal then it aborts.
>
>> I can't imagine how to do it, as someone should remove hint from error_abort object on
>> success path..
>>
>> But seems, the following is possible, which seems better for me than local-error approach:
>>
>> error_push_hint(errp, hint);
>> ret = f(.., errp);
>> error_pop_hint(errp);
>>
>
> Matter of taste... also, it looks awkward to come up with a hint
> before knowing what happened. I mean the appropriate hint could
> depend on the value returned by f() and/or errno for example.
>
>> ===
>>
>> Continue thinking on this:
>>
>> It may look like just
>> ret = f(..., set_hint(errp, hint));
>>
>> or (just to split long line):
>> set_hint(errp, hint);
>> ret = f(..., errp);
>>
>> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
>> which may be just one call at function start of macro, which will call error_push_hint(errp) and
>> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
>> exit..
>>
>> Or, may be, more direct way to set cleanup for function exists?
>>
>> ===
>>
>> Also, we can implement some code generation, to generate for functions with errp argument
>> wrappers with additional hint parameter, and just use these wrappers..
>>
>> ===
>>
>> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
>> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
>>
>
> For the reason exposed above, I don't think it makes sense to
> build the hint before calling a function that calls error_setg().
> I'm afraid we're stuck with local_err... it is then up to the
> people to make it as less painful as possible.
>
Hmm. so, seems that in general we need local_err..
Then may be, may can make automated propagation?
It will look like
g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
.errp = errp,
.local_err = NULL,
}
errp = &_error_prop.local_err;
and this thing may be fully covered into macro,
to look like this at function start (to be honest it should exactly after all
local variable definitions):
MAKE_ERRP_SAFE(_error_prop, errp);
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 17:40 ` Vladimir Sementsov-Ogievskiy
@ 2019-09-18 7:58 ` Greg Kurz
2019-09-18 10:17 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-18 7:58 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Max Reitz, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, Richard Henderson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, David Gibson, Kevin Wolf, berrange, Cornelia Huck,
qemu-s390x, Subbaraya Sundeep, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 17:40:11 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 17.09.2019 18:37, Greg Kurz wrote:
> > On Tue, 17 Sep 2019 13:25:03 +0000
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> >
> >> 17.09.2019 13:20, Greg Kurz wrote:
> >>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >>>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>> block/backup.c | 7 +++++--
> >>> block/dirty-bitmap.c | 7 +++++--
> >>> block/file-posix.c | 20 +++++++++++++-------
> >>> block/gluster.c | 23 +++++++++++++++--------
> >>> block/qcow.c | 10 ++++++----
> >>> block/qcow2.c | 7 +++++--
> >>> block/vhdx-log.c | 7 +++++--
> >>> block/vpc.c | 7 +++++--
> >>> 8 files changed, 59 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/block/backup.c b/block/backup.c
> >>> index 763f0d7ff6db..d8c422a0e3bc 100644
> >>> --- a/block/backup.c
> >>> +++ b/block/backup.c
> >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> >>> BACKUP_CLUSTER_SIZE_DEFAULT);
> >>> return BACKUP_CLUSTER_SIZE_DEFAULT;
> >>> } else if (ret < 0 && !target->backing) {
> >>> - error_setg_errno(errp, -ret,
> >>> + Error *local_err = NULL;
> >>> +
> >>> + error_setg_errno(&local_err, -ret,
> >>> "Couldn't determine the cluster size of the target image, "
> >>> "which has no backing file");
> >>> - error_append_hint(errp,
> >>> + error_append_hint(&local_err,
> >>> "Aborting, since this may create an unusable destination image\n");
> >>> + error_propagate(errp, local_err);
> >>> return ret;
> >>> } else if (ret < 0 && target->backing) {
> >>> /* Not fatal; just trudge on ahead. */
> >>
> >>
> >> Pain.. Do we need these hints, if they are so painful?
> >>
> >
> > I agree that the one above doesn't qualify as a useful hint.
> > It just tells that QEMU is giving up and gives no indication
> > to the user on how to avoid the issue. It should probably be
> > dropped.
> >
> >> At least for cases like this, we can create helper function
> >>
> >> error_setg_errno_hint(..., error, hint)
> >
> > Not very useful if hint has to be forged separately with
> > g_sprintf(), and we can't have such a thing as:
> >
> > error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
> >
> >>
> >> But what could be done when we call function, which may or may not set errp?
> >>
> >> ret = f(errp);
> >> if (ret) {
> >> error_append_hint(errp, hint);
> >> }
> >>
> >
> > Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
> > ends up calling exit().
> >
> >> Hmmm..
> >>
> >> Can it look like
> >>
> >> ret = f(..., hint_helper(errp, hint))
> >>
> >> ?
> >>
> >
> > Nope, hint_helper() will get called before f() and things are worse.
> > If errp is NULL then error_append_hint() does nothing and if it is
> > &error_fatal then it aborts.
> >
> >> I can't imagine how to do it, as someone should remove hint from error_abort object on
> >> success path..
> >>
> >> But seems, the following is possible, which seems better for me than local-error approach:
> >>
> >> error_push_hint(errp, hint);
> >> ret = f(.., errp);
> >> error_pop_hint(errp);
> >>
> >
> > Matter of taste... also, it looks awkward to come up with a hint
> > before knowing what happened. I mean the appropriate hint could
> > depend on the value returned by f() and/or errno for example.
> >
> >> ===
> >>
> >> Continue thinking on this:
> >>
> >> It may look like just
> >> ret = f(..., set_hint(errp, hint));
> >>
> >> or (just to split long line):
> >> set_hint(errp, hint);
> >> ret = f(..., errp);
> >>
> >> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
> >> which may be just one call at function start of macro, which will call error_push_hint(errp) and
> >> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
> >> exit..
> >>
> >> Or, may be, more direct way to set cleanup for function exists?
> >>
> >> ===
> >>
> >> Also, we can implement some code generation, to generate for functions with errp argument
> >> wrappers with additional hint parameter, and just use these wrappers..
> >>
> >> ===
> >>
> >> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
> >> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
> >>
> >
> > For the reason exposed above, I don't think it makes sense to
> > build the hint before calling a function that calls error_setg().
> > I'm afraid we're stuck with local_err... it is then up to the
> > people to make it as less painful as possible.
> >
>
> Hmm. so, seems that in general we need local_err..
>
> Then may be, may can make automated propagation?
>
> It will look like
>
> g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
> .errp = errp,
> .local_err = NULL,
> }
>
> errp = &_error_prop.local_err;
>
> and this thing may be fully covered into macro,
> to look like this at function start (to be honest it should exactly after all
> local variable definitions):
>
> MAKE_ERRP_SAFE(_error_prop, errp);
>
>
Maybe you can send an RFC patch that converts a handful of
local_err users to g_auto() ?
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-18 7:58 ` Greg Kurz
@ 2019-09-18 10:17 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 49+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-18 10:17 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Max Reitz, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, Richard Henderson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, David Gibson, Kevin Wolf, berrange, Cornelia Huck,
qemu-s390x, Subbaraya Sundeep, qemu-ppc, Paolo Bonzini
18.09.2019 10:58, Greg Kurz wrote:
> On Tue, 17 Sep 2019 17:40:11 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> 17.09.2019 18:37, Greg Kurz wrote:
>>> On Tue, 17 Sep 2019 13:25:03 +0000
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> 17.09.2019 13:20, Greg Kurz wrote:
>>>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>>>
>>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>>> ---
>>>>> block/backup.c | 7 +++++--
>>>>> block/dirty-bitmap.c | 7 +++++--
>>>>> block/file-posix.c | 20 +++++++++++++-------
>>>>> block/gluster.c | 23 +++++++++++++++--------
>>>>> block/qcow.c | 10 ++++++----
>>>>> block/qcow2.c | 7 +++++--
>>>>> block/vhdx-log.c | 7 +++++--
>>>>> block/vpc.c | 7 +++++--
>>>>> 8 files changed, 59 insertions(+), 29 deletions(-)
>>>>>
>>>>> diff --git a/block/backup.c b/block/backup.c
>>>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>>>> --- a/block/backup.c
>>>>> +++ b/block/backup.c
>>>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>>>> BACKUP_CLUSTER_SIZE_DEFAULT);
>>>>> return BACKUP_CLUSTER_SIZE_DEFAULT;
>>>>> } else if (ret < 0 && !target->backing) {
>>>>> - error_setg_errno(errp, -ret,
>>>>> + Error *local_err = NULL;
>>>>> +
>>>>> + error_setg_errno(&local_err, -ret,
>>>>> "Couldn't determine the cluster size of the target image, "
>>>>> "which has no backing file");
>>>>> - error_append_hint(errp,
>>>>> + error_append_hint(&local_err,
>>>>> "Aborting, since this may create an unusable destination image\n");
>>>>> + error_propagate(errp, local_err);
>>>>> return ret;
>>>>> } else if (ret < 0 && target->backing) {
>>>>> /* Not fatal; just trudge on ahead. */
>>>>
>>>>
>>>> Pain.. Do we need these hints, if they are so painful?
>>>>
>>>
>>> I agree that the one above doesn't qualify as a useful hint.
>>> It just tells that QEMU is giving up and gives no indication
>>> to the user on how to avoid the issue. It should probably be
>>> dropped.
>>>
>>>> At least for cases like this, we can create helper function
>>>>
>>>> error_setg_errno_hint(..., error, hint)
>>>
>>> Not very useful if hint has to be forged separately with
>>> g_sprintf(), and we can't have such a thing as:
>>>
>>> error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
>>>
>>>>
>>>> But what could be done when we call function, which may or may not set errp?
>>>>
>>>> ret = f(errp);
>>>> if (ret) {
>>>> error_append_hint(errp, hint);
>>>> }
>>>>
>>>
>>> Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
>>> ends up calling exit().
>>>
>>>> Hmmm..
>>>>
>>>> Can it look like
>>>>
>>>> ret = f(..., hint_helper(errp, hint))
>>>>
>>>> ?
>>>>
>>>
>>> Nope, hint_helper() will get called before f() and things are worse.
>>> If errp is NULL then error_append_hint() does nothing and if it is
>>> &error_fatal then it aborts.
>>>
>>>> I can't imagine how to do it, as someone should remove hint from error_abort object on
>>>> success path..
>>>>
>>>> But seems, the following is possible, which seems better for me than local-error approach:
>>>>
>>>> error_push_hint(errp, hint);
>>>> ret = f(.., errp);
>>>> error_pop_hint(errp);
>>>>
>>>
>>> Matter of taste... also, it looks awkward to come up with a hint
>>> before knowing what happened. I mean the appropriate hint could
>>> depend on the value returned by f() and/or errno for example.
>>>
>>>> ===
>>>>
>>>> Continue thinking on this:
>>>>
>>>> It may look like just
>>>> ret = f(..., set_hint(errp, hint));
>>>>
>>>> or (just to split long line):
>>>> set_hint(errp, hint);
>>>> ret = f(..., errp);
>>>>
>>>> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
>>>> which may be just one call at function start of macro, which will call error_push_hint(errp) and
>>>> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
>>>> exit..
>>>>
>>>> Or, may be, more direct way to set cleanup for function exists?
>>>>
>>>> ===
>>>>
>>>> Also, we can implement some code generation, to generate for functions with errp argument
>>>> wrappers with additional hint parameter, and just use these wrappers..
>>>>
>>>> ===
>>>>
>>>> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
>>>> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
>>>>
>>>
>>> For the reason exposed above, I don't think it makes sense to
>>> build the hint before calling a function that calls error_setg().
>>> I'm afraid we're stuck with local_err... it is then up to the
>>> people to make it as less painful as possible.
>>>
>>
>> Hmm. so, seems that in general we need local_err..
>>
>> Then may be, may can make automated propagation?
>>
>> It will look like
>>
>> g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
>> .errp = errp,
>> .local_err = NULL,
>> }
>>
>> errp = &_error_prop.local_err;
>>
>> and this thing may be fully covered into macro,
>> to look like this at function start (to be honest it should exactly after all
>> local variable definitions):
>>
>> MAKE_ERRP_SAFE(_error_prop, errp);
>>
>>
>
> Maybe you can send an RFC patch that converts a handful of
> local_err users to g_auto() ?
>
Yes, I'll try
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 ` [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint() Greg Kurz
2019-09-17 13:25 ` Vladimir Sementsov-Ogievskiy
@ 2019-09-17 14:39 ` Eric Blake
2019-09-17 14:46 ` Kevin Wolf
1 sibling, 1 reply; 49+ messages in thread
From: Eric Blake @ 2019-09-17 14:39 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
qemu-block, Juan Quintela, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, qemu-arm, John Snow, Richard Henderson,
Kevin Wolf, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On 9/17/19 5:20 AM, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> block/backup.c | 7 +++++--
> block/dirty-bitmap.c | 7 +++++--
> block/file-posix.c | 20 +++++++++++++-------
> block/gluster.c | 23 +++++++++++++++--------
> block/qcow.c | 10 ++++++----
> block/qcow2.c | 7 +++++--
> block/vhdx-log.c | 7 +++++--
> block/vpc.c | 7 +++++--
> 8 files changed, 59 insertions(+), 29 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6db..d8c422a0e3bc 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> BACKUP_CLUSTER_SIZE_DEFAULT);
> return BACKUP_CLUSTER_SIZE_DEFAULT;
> } else if (ret < 0 && !target->backing) {
> - error_setg_errno(errp, -ret,
> + Error *local_err = NULL;
Can we go with the shorter name 'err' instead of 'local_err'? I know,
we aren't consistent (both styles appear throughout the tree), but the
shorter style is more appealing to me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 14:39 ` Eric Blake
@ 2019-09-17 14:46 ` Kevin Wolf
2019-09-17 16:40 ` Greg Kurz
2019-09-17 19:10 ` John Snow
0 siblings, 2 replies; 49+ messages in thread
From: Kevin Wolf @ 2019-09-17 14:46 UTC (permalink / raw)
To: Eric Blake
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman, Greg Kurz,
Yuval Shaia, Dr. David Alan Gilbert, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> On 9/17/19 5:20 AM, Greg Kurz wrote:
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > block/backup.c | 7 +++++--
> > block/dirty-bitmap.c | 7 +++++--
> > block/file-posix.c | 20 +++++++++++++-------
> > block/gluster.c | 23 +++++++++++++++--------
> > block/qcow.c | 10 ++++++----
> > block/qcow2.c | 7 +++++--
> > block/vhdx-log.c | 7 +++++--
> > block/vpc.c | 7 +++++--
> > 8 files changed, 59 insertions(+), 29 deletions(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index 763f0d7ff6db..d8c422a0e3bc 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> > BACKUP_CLUSTER_SIZE_DEFAULT);
> > return BACKUP_CLUSTER_SIZE_DEFAULT;
> > } else if (ret < 0 && !target->backing) {
> > - error_setg_errno(errp, -ret,
> > + Error *local_err = NULL;
>
> Can we go with the shorter name 'err' instead of 'local_err'? I know,
> we aren't consistent (both styles appear throughout the tree), but the
> shorter style is more appealing to me.
I like local_err better because it's easier to distinguish from errp.
The compiler might catch it if you use the wrong one because one is
Error* and the other is Error**, but as a reviewer, I can still get
confused.
Kevin
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 14:46 ` Kevin Wolf
@ 2019-09-17 16:40 ` Greg Kurz
2019-09-17 19:10 ` John Snow
1 sibling, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 16:40 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 16:46:31 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> > On 9/17/19 5:20 AM, Greg Kurz wrote:
> > > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > block/backup.c | 7 +++++--
> > > block/dirty-bitmap.c | 7 +++++--
> > > block/file-posix.c | 20 +++++++++++++-------
> > > block/gluster.c | 23 +++++++++++++++--------
> > > block/qcow.c | 10 ++++++----
> > > block/qcow2.c | 7 +++++--
> > > block/vhdx-log.c | 7 +++++--
> > > block/vpc.c | 7 +++++--
> > > 8 files changed, 59 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/block/backup.c b/block/backup.c
> > > index 763f0d7ff6db..d8c422a0e3bc 100644
> > > --- a/block/backup.c
> > > +++ b/block/backup.c
> > > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> > > BACKUP_CLUSTER_SIZE_DEFAULT);
> > > return BACKUP_CLUSTER_SIZE_DEFAULT;
> > > } else if (ret < 0 && !target->backing) {
> > > - error_setg_errno(errp, -ret,
> > > + Error *local_err = NULL;
> >
> > Can we go with the shorter name 'err' instead of 'local_err'? I know,
> > we aren't consistent (both styles appear throughout the tree), but the
> > shorter style is more appealing to me.
>
> I like local_err better because it's easier to distinguish from errp.
> The compiler might catch it if you use the wrong one because one is
> Error* and the other is Error**, but as a reviewer, I can still get
> confused.
>
I'll favor the official maintainer, hence keeping local_err :)
> Kevin
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 14:46 ` Kevin Wolf
2019-09-17 16:40 ` Greg Kurz
@ 2019-09-17 19:10 ` John Snow
2019-09-18 7:33 ` Kevin Wolf
1 sibling, 1 reply; 49+ messages in thread
From: John Snow @ 2019-09-17 19:10 UTC (permalink / raw)
To: Kevin Wolf, Eric Blake
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman, Greg Kurz,
Yuval Shaia, Dr. David Alan Gilbert, Alex Williamson, qemu-arm,
Richard Henderson, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On 9/17/19 10:46 AM, Kevin Wolf wrote:
> Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
>> On 9/17/19 5:20 AM, Greg Kurz wrote:
>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>> block/backup.c | 7 +++++--
>>> block/dirty-bitmap.c | 7 +++++--
>>> block/file-posix.c | 20 +++++++++++++-------
>>> block/gluster.c | 23 +++++++++++++++--------
>>> block/qcow.c | 10 ++++++----
>>> block/qcow2.c | 7 +++++--
>>> block/vhdx-log.c | 7 +++++--
>>> block/vpc.c | 7 +++++--
>>> 8 files changed, 59 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>> BACKUP_CLUSTER_SIZE_DEFAULT);
>>> return BACKUP_CLUSTER_SIZE_DEFAULT;
>>> } else if (ret < 0 && !target->backing) {
>>> - error_setg_errno(errp, -ret,
>>> + Error *local_err = NULL;
>>
>> Can we go with the shorter name 'err' instead of 'local_err'? I know,
>> we aren't consistent (both styles appear throughout the tree), but the
>> shorter style is more appealing to me.
>
> I like local_err better because it's easier to distinguish from errp.
> The compiler might catch it if you use the wrong one because one is
> Error* and the other is Error**, but as a reviewer, I can still get
> confused.
>
> Kevin
>
Doesn't that sound like a striking reason for condemnation of this
entire model?
What's going to prevent us from regressing on this technicality in the
future?
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
2019-09-17 19:10 ` John Snow
@ 2019-09-18 7:33 ` Kevin Wolf
0 siblings, 0 replies; 49+ messages in thread
From: Kevin Wolf @ 2019-09-18 7:33 UTC (permalink / raw)
To: John Snow
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman, Greg Kurz,
Yuval Shaia, Dr. David Alan Gilbert, Alex Williamson, qemu-arm,
Richard Henderson, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
Am 17.09.2019 um 21:10 hat John Snow geschrieben:
>
>
> On 9/17/19 10:46 AM, Kevin Wolf wrote:
> > Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> >> On 9/17/19 5:20 AM, Greg Kurz wrote:
> >>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >>>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>> block/backup.c | 7 +++++--
> >>> block/dirty-bitmap.c | 7 +++++--
> >>> block/file-posix.c | 20 +++++++++++++-------
> >>> block/gluster.c | 23 +++++++++++++++--------
> >>> block/qcow.c | 10 ++++++----
> >>> block/qcow2.c | 7 +++++--
> >>> block/vhdx-log.c | 7 +++++--
> >>> block/vpc.c | 7 +++++--
> >>> 8 files changed, 59 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/block/backup.c b/block/backup.c
> >>> index 763f0d7ff6db..d8c422a0e3bc 100644
> >>> --- a/block/backup.c
> >>> +++ b/block/backup.c
> >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> >>> BACKUP_CLUSTER_SIZE_DEFAULT);
> >>> return BACKUP_CLUSTER_SIZE_DEFAULT;
> >>> } else if (ret < 0 && !target->backing) {
> >>> - error_setg_errno(errp, -ret,
> >>> + Error *local_err = NULL;
> >>
> >> Can we go with the shorter name 'err' instead of 'local_err'? I know,
> >> we aren't consistent (both styles appear throughout the tree), but the
> >> shorter style is more appealing to me.
> >
> > I like local_err better because it's easier to distinguish from errp.
> > The compiler might catch it if you use the wrong one because one is
> > Error* and the other is Error**, but as a reviewer, I can still get
> > confused.
>
> Doesn't that sound like a striking reason for condemnation of this
> entire model?
Might be, but do you have a better idea?
Kevin
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Qemu-devel] [PATCH 03/17] char/spice: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
2019-09-17 10:20 ` [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation Greg Kurz
2019-09-17 10:20 ` [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint() Greg Kurz
@ 2019-09-17 10:20 ` Greg Kurz
2019-09-17 10:20 ` [Qemu-devel] [PATCH 04/17] ppc: " Greg Kurz
` (14 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, Mark Cave-Ayland, Michael Roth,
Gerd Hoffmann, Subbaraya Sundeep, Juan Quintela,
David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
chardev/spice.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/chardev/spice.c b/chardev/spice.c
index 241e2b7770eb..ed5996467c32 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -279,10 +279,12 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
if (*psubtype == NULL) {
char *subtypes = g_strjoinv(", ",
(gchar **)spice_server_char_device_recognized_subtypes());
+ Error *local_err = NULL;
- error_setg(errp, "unsupported type name: %s", type);
- error_append_hint(errp, "allowed spice char type names: %s\n",
+ error_setg(&local_err, "unsupported type name: %s", type);
+ error_append_hint(&local_err, "allowed spice char type names: %s\n",
subtypes);
+ error_propagate(errp, local_err);
g_free(subtypes);
return;
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Qemu-devel] [PATCH 04/17] ppc: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (2 preceding siblings ...)
2019-09-17 10:20 ` [Qemu-devel] [PATCH 03/17] char/spice: " Greg Kurz
@ 2019-09-17 10:20 ` Greg Kurz
2019-09-18 0:12 ` David Gibson
2019-09-17 10:21 ` [Qemu-devel] [PATCH 05/17] arm: " Greg Kurz
` (13 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:20 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, Eric Farman, Dr. David Alan Gilbert,
Yuval Shaia, Alex Williamson, John Snow, Richard Henderson,
Kevin Wolf, Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/mac_newworld.c | 7 +++++--
hw/ppc/spapr.c | 7 +++++--
hw/ppc/spapr_pci.c | 9 +++++----
target/ppc/kvm.c | 13 +++++++++----
4 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index c5bbcc743352..aca8c40bf395 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -618,8 +618,11 @@ static void core99_set_via_config(Object *obj, const char *value, Error **errp)
} else if (!strcmp(value, "pmu-adb")) {
cms->via_config = CORE99_VIA_CONFIG_PMU_ADB;
} else {
- error_setg(errp, "Invalid via value");
- error_append_hint(errp, "Valid values are cuda, pmu, pmu-adb.\n");
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Invalid via value");
+ error_append_hint(&local_err, "Valid values are cuda, pmu, pmu-adb.\n");
+ error_propagate(errp, local_err);
}
}
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 08a2a5a77092..39d6f57d014e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4337,10 +4337,13 @@ void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
vcpu_id = spapr_vcpu_id(spapr, cpu_index);
if (kvm_enabled() && !kvm_vcpu_id_is_valid(vcpu_id)) {
- error_setg(errp, "Can't create CPU with id %d in KVM", vcpu_id);
- error_append_hint(errp, "Adjust the number of cpus to %d "
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Can't create CPU with id %d in KVM", vcpu_id);
+ error_append_hint(&local_err, "Adjust the number of cpus to %d "
"or try to raise the number of threads per core\n",
vcpu_id * ms->smp.threads / spapr->vsmt);
+ error_propagate(errp, local_err);
return;
}
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7b71ad7c74f1..4b7e9a1c8666 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1870,12 +1870,13 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
if (spapr_pci_find_phb(spapr, sphb->buid)) {
SpaprPhbState *s;
- error_setg(errp, "PCI host bridges must have unique indexes");
- error_append_hint(errp, "The following indexes are already in use:");
+ error_setg(&local_err, "PCI host bridges must have unique indexes");
+ error_append_hint(&local_err, "The following indexes are already in use:");
QLIST_FOREACH(s, &spapr->phbs, list) {
- error_append_hint(errp, " %d", s->index);
+ error_append_hint(&local_err, " %d", s->index);
}
- error_append_hint(errp, "\nTry another value for the index property\n");
+ error_append_hint(&local_err, "\nTry another value for the index property\n");
+ error_propagate(errp, local_err);
return;
}
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 8c5b1f25cc95..c6876b08c726 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -242,8 +242,11 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
assert(kvm_state != NULL);
if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_GET_SMMU_INFO)) {
- error_setg(errp, "KVM doesn't expose the MMU features it supports");
- error_append_hint(errp, "Consider switching to a newer KVM\n");
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "KVM doesn't expose the MMU features it supports");
+ error_append_hint(&local_err, "Consider switching to a newer KVM\n");
+ error_propagate(errp, local_err);
return;
}
@@ -2076,6 +2079,7 @@ void kvmppc_hint_smt_possible(Error **errp)
int i;
GString *g;
char *s;
+ Error *local_err = NULL;
assert(kvm_enabled());
if (cap_ppc_smt_possible) {
@@ -2086,12 +2090,13 @@ void kvmppc_hint_smt_possible(Error **errp)
}
}
s = g_string_free(g, false);
- error_append_hint(errp, "%s.\n", s);
+ error_append_hint(&local_err, "%s.\n", s);
g_free(s);
} else {
- error_append_hint(errp,
+ error_append_hint(&local_err,
"This KVM seems to be too old to support VSMT.\n");
}
+ error_propagate(errp, local_err);
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 04/17] ppc: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 ` [Qemu-devel] [PATCH 04/17] ppc: " Greg Kurz
@ 2019-09-18 0:12 ` David Gibson
0 siblings, 0 replies; 49+ messages in thread
From: David Gibson @ 2019-09-18 0:12 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, Eric Farman, Dr. David Alan Gilbert,
Yuval Shaia, Alex Williamson, John Snow, Richard Henderson,
Kevin Wolf, Cornelia Huck, Max Reitz, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 5049 bytes --]
On Tue, Sep 17, 2019 at 12:20:56PM +0200, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/mac_newworld.c | 7 +++++--
> hw/ppc/spapr.c | 7 +++++--
> hw/ppc/spapr_pci.c | 9 +++++----
> target/ppc/kvm.c | 13 +++++++++----
> 4 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index c5bbcc743352..aca8c40bf395 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -618,8 +618,11 @@ static void core99_set_via_config(Object *obj, const char *value, Error **errp)
> } else if (!strcmp(value, "pmu-adb")) {
> cms->via_config = CORE99_VIA_CONFIG_PMU_ADB;
> } else {
> - error_setg(errp, "Invalid via value");
> - error_append_hint(errp, "Valid values are cuda, pmu, pmu-adb.\n");
> + Error *local_err = NULL;
> +
> + error_setg(&local_err, "Invalid via value");
> + error_append_hint(&local_err, "Valid values are cuda, pmu, pmu-adb.\n");
> + error_propagate(errp, local_err);
> }
> }
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 08a2a5a77092..39d6f57d014e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4337,10 +4337,13 @@ void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
> vcpu_id = spapr_vcpu_id(spapr, cpu_index);
>
> if (kvm_enabled() && !kvm_vcpu_id_is_valid(vcpu_id)) {
> - error_setg(errp, "Can't create CPU with id %d in KVM", vcpu_id);
> - error_append_hint(errp, "Adjust the number of cpus to %d "
> + Error *local_err = NULL;
> +
> + error_setg(&local_err, "Can't create CPU with id %d in KVM", vcpu_id);
> + error_append_hint(&local_err, "Adjust the number of cpus to %d "
> "or try to raise the number of threads per core\n",
> vcpu_id * ms->smp.threads / spapr->vsmt);
> + error_propagate(errp, local_err);
> return;
> }
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7b71ad7c74f1..4b7e9a1c8666 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1870,12 +1870,13 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> if (spapr_pci_find_phb(spapr, sphb->buid)) {
> SpaprPhbState *s;
>
> - error_setg(errp, "PCI host bridges must have unique indexes");
> - error_append_hint(errp, "The following indexes are already in use:");
> + error_setg(&local_err, "PCI host bridges must have unique indexes");
> + error_append_hint(&local_err, "The following indexes are already in use:");
> QLIST_FOREACH(s, &spapr->phbs, list) {
> - error_append_hint(errp, " %d", s->index);
> + error_append_hint(&local_err, " %d", s->index);
> }
> - error_append_hint(errp, "\nTry another value for the index property\n");
> + error_append_hint(&local_err, "\nTry another value for the index property\n");
> + error_propagate(errp, local_err);
> return;
> }
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 8c5b1f25cc95..c6876b08c726 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -242,8 +242,11 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
> assert(kvm_state != NULL);
>
> if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_GET_SMMU_INFO)) {
> - error_setg(errp, "KVM doesn't expose the MMU features it supports");
> - error_append_hint(errp, "Consider switching to a newer KVM\n");
> + Error *local_err = NULL;
> +
> + error_setg(&local_err, "KVM doesn't expose the MMU features it supports");
> + error_append_hint(&local_err, "Consider switching to a newer KVM\n");
> + error_propagate(errp, local_err);
> return;
> }
>
> @@ -2076,6 +2079,7 @@ void kvmppc_hint_smt_possible(Error **errp)
> int i;
> GString *g;
> char *s;
> + Error *local_err = NULL;
>
> assert(kvm_enabled());
> if (cap_ppc_smt_possible) {
> @@ -2086,12 +2090,13 @@ void kvmppc_hint_smt_possible(Error **errp)
> }
> }
> s = g_string_free(g, false);
> - error_append_hint(errp, "%s.\n", s);
> + error_append_hint(&local_err, "%s.\n", s);
> g_free(s);
> } else {
> - error_append_hint(errp,
> + error_append_hint(&local_err,
> "This KVM seems to be too old to support VSMT.\n");
> }
> + error_propagate(errp, local_err);
> }
>
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Qemu-devel] [PATCH 05/17] arm: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (3 preceding siblings ...)
2019-09-17 10:20 ` [Qemu-devel] [PATCH 04/17] ppc: " Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 10:21 ` [Qemu-devel] [PATCH 06/17] vfio: " Greg Kurz
` (12 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/arm/msf2-soc.c | 5 +++--
hw/arm/virt.c | 14 ++++++++++----
hw/intc/arm_gicv3_kvm.c | 5 +++--
hw/misc/msf2-sysreg.c | 6 ++++--
4 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index 008fd9327aa4..f606cfe7d139 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -127,8 +127,9 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
}
if (!s->m3clk) {
- error_setg(errp, "Invalid m3clk value");
- error_append_hint(errp, "m3clk can not be zero\n");
+ error_setg(&err, "Invalid m3clk value");
+ error_append_hint(&err, "m3clk can not be zero\n");
+ error_propagate(errp, err);
return;
}
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d74538b0212e..7ac00dab581e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1804,8 +1804,11 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
} else if (!strcmp(value, "max")) {
vms->gic_version = -1; /* Will probe later */
} else {
- error_setg(errp, "Invalid gic-version value");
- error_append_hint(errp, "Valid values are 3, 2, host, max.\n");
+ Error *err = NULL;
+
+ error_setg(&err, "Invalid gic-version value");
+ error_append_hint(&err, "Valid values are 3, 2, host, max.\n");
+ error_propagate(errp, err);
}
}
@@ -1832,8 +1835,11 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
} else if (!strcmp(value, "none")) {
vms->iommu = VIRT_IOMMU_NONE;
} else {
- error_setg(errp, "Invalid iommu value");
- error_append_hint(errp, "Valid values are none, smmuv3.\n");
+ Error *err = NULL;
+
+ error_setg(&err, "Invalid iommu value");
+ error_append_hint(&err, "Valid values are none, smmuv3.\n");
+ error_propagate(errp, err);
}
}
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 9c7f4ab8711c..09784f06f9ad 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -810,10 +810,11 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION);
if (!multiple_redist_region_allowed && s->nb_redist_regions > 1) {
- error_setg(errp, "Multiple VGICv3 redistributor regions are not "
+ error_setg(&local_err, "Multiple VGICv3 redistributor regions are not "
"supported by this host kernel");
- error_append_hint(errp, "A maximum of %d VCPUs can be used",
+ error_append_hint(&local_err, "A maximum of %d VCPUs can be used",
s->redist_region_count[0]);
+ error_propagate(errp, local_err);
return;
}
diff --git a/hw/misc/msf2-sysreg.c b/hw/misc/msf2-sysreg.c
index ddc5a30c80f7..23c7890ac022 100644
--- a/hw/misc/msf2-sysreg.c
+++ b/hw/misc/msf2-sysreg.c
@@ -128,12 +128,14 @@ static Property msf2_sysreg_properties[] = {
static void msf2_sysreg_realize(DeviceState *dev, Error **errp)
{
MSF2SysregState *s = MSF2_SYSREG(dev);
+ Error *local_err = NULL;
if ((s->apb0div > 32 || !is_power_of_2(s->apb0div))
|| (s->apb1div > 32 || !is_power_of_2(s->apb1div))) {
- error_setg(errp, "Invalid apb divisor value");
- error_append_hint(errp, "apb divisor must be a power of 2"
+ error_setg(&local_err, "Invalid apb divisor value");
+ error_append_hint(&local_err, "apb divisor must be a power of 2"
" and maximum value is 32\n");
+ error_propagate(errp, local_err);
}
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Qemu-devel] [PATCH 06/17] vfio: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (4 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 05/17] arm: " Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 11:26 ` Cornelia Huck
2019-09-17 10:21 ` [Qemu-devel] [PATCH 07/17] virtio-pci: " Greg Kurz
` (11 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/vfio/common.c | 14 ++++++++++----
hw/vfio/pci.c | 12 ++++++++----
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 3e03c495d868..05b1c39a426f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1469,10 +1469,13 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
}
if (!(status.flags & VFIO_GROUP_FLAGS_VIABLE)) {
- error_setg(errp, "group %d is not viable", groupid);
- error_append_hint(errp,
+ Error *err = NULL;
+
+ error_setg(&err, "group %d is not viable", groupid);
+ error_append_hint(&err,
"Please ensure all devices within the iommu_group "
"are bound to their vfio bus driver.\n");
+ error_propagate(errp, err);
goto close_fd_exit;
}
@@ -1531,11 +1534,14 @@ int vfio_get_device(VFIOGroup *group, const char *name,
fd = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name);
if (fd < 0) {
- error_setg_errno(errp, errno, "error getting device from group %d",
+ Error *err = NULL;
+
+ error_setg_errno(&err, errno, "error getting device from group %d",
group->groupid);
- error_append_hint(errp,
+ error_append_hint(&err,
"Verify all devices in group %d are bound to vfio-<bus> "
"or pci-stub and not already in use\n", group->groupid);
+ error_propagate(errp, err);
return fd;
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index dc3479c374e3..b037dec2215b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2527,10 +2527,13 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
g_free(reg_info);
if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
- ret = vfio_populate_vga(vdev, errp);
+ Error *err = NULL;
+
+ ret = vfio_populate_vga(vdev, &err);
if (ret) {
- error_append_hint(errp, "device does not support "
+ error_append_hint(&err, "device does not support "
"requested feature x-vga\n");
+ error_propagate(errp, err);
return;
}
}
@@ -2714,9 +2717,10 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
if (!vdev->vbasedev.sysfsdev) {
if (!(~vdev->host.domain || ~vdev->host.bus ||
~vdev->host.slot || ~vdev->host.function)) {
- error_setg(errp, "No provided host device");
- error_append_hint(errp, "Use -device vfio-pci,host=DDDD:BB:DD.F "
+ error_setg(&err, "No provided host device");
+ error_append_hint(&err, "Use -device vfio-pci,host=DDDD:BB:DD.F "
"or -device vfio-pci,sysfsdev=PATH_TO_DEVICE\n");
+ error_propagate(errp, err);
return;
}
vdev->vbasedev.sysfsdev =
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 06/17] vfio: Pass local error object pointer to error_append_hint()
2019-09-17 10:21 ` [Qemu-devel] [PATCH 06/17] vfio: " Greg Kurz
@ 2019-09-17 11:26 ` Cornelia Huck
0 siblings, 0 replies; 49+ messages in thread
From: Cornelia Huck @ 2019-09-17 11:26 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 12:21:08 +0200
Greg Kurz <groug@kaod.org> wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/vfio/common.c | 14 ++++++++++----
> hw/vfio/pci.c | 12 ++++++++----
> 2 files changed, 18 insertions(+), 8 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Qemu-devel] [PATCH 07/17] virtio-pci: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (5 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 06/17] vfio: " Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 10:21 ` [Qemu-devel] [PATCH 08/17] pcie_root_port: " Greg Kurz
` (10 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, John Snow,
Richard Henderson, Kevin Wolf, Cornelia Huck, Max Reitz,
Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/virtio/virtio-pci.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c6b47a9c7385..1e58822ba186 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1543,9 +1543,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
virtio_pci_disable_modern(proxy);
if (!legacy) {
- error_setg(errp, "Device doesn't support modern mode, and legacy"
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Device doesn't support modern mode, and legacy"
" mode is disabled");
- error_append_hint(errp, "Set disable-legacy to off\n");
+ error_append_hint(&local_err, "Set disable-legacy to off\n");
+ error_propagate(errp, local_err);
return;
}
@@ -1737,10 +1740,13 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
}
if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
- error_setg(errp, "device cannot work as neither modern nor legacy mode"
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "device cannot work as neither modern nor legacy mode"
" is enabled");
- error_append_hint(errp, "Set either disable-modern or disable-legacy"
+ error_append_hint(&local_err, "Set either disable-modern or disable-legacy"
" to off\n");
+ error_propagate(errp, local_err);
return;
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Qemu-devel] [PATCH 08/17] pcie_root_port: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (6 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 07/17] virtio-pci: " Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 10:21 ` [Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report() Greg Kurz
` (9 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/pci-bridge/pcie_root_port.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 012c2cb12c10..086f9c17723d 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -65,15 +65,17 @@ static void rp_realize(PCIDevice *d, Error **errp)
PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
int rc;
+ Error *local_err = NULL;
pci_config_set_interrupt_pin(d->config, 1);
pci_bridge_initfn(d, TYPE_PCIE_BUS);
pcie_port_init_reg(d);
rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
- rpc->ssid, errp);
+ rpc->ssid, &local_err);
if (rc < 0) {
- error_append_hint(errp, "Can't init SSV ID, error %d\n", rc);
+ error_append_hint(&local_err, "Can't init SSV ID, error %d\n", rc);
+ error_propagate(errp, local_err);
goto err_bridge;
}
@@ -85,10 +87,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
}
rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
- p->port, errp);
+ p->port, &local_err);
if (rc < 0) {
- error_append_hint(errp, "Can't add Root Port capability, "
+ error_append_hint(&local_err, "Can't add Root Port capability, "
"error %d\n", rc);
+ error_propagate(errp, local_err);
goto err_int;
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (7 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 08/17] pcie_root_port: " Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 14:51 ` Yuval Shaia
2019-09-17 10:21 ` [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint() Greg Kurz
` (8 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Commit 4d71b38ae8fa converted many error_setg() call sites to
rdma_error_report(), but it forgot to convert a companion
error_append_hint(). Since no guy doesn't set errp anymore in
pvrdma_realize(), errp remains NULL and error_append_hint() does
nothing.
Also error_append_hint() was a poor choice since its "intended use
is adding helpful hints on the human user interface" and "not for
clarifying a confusing error message".
Call rdma_error_report() instead.
Fixes: 4d71b38ae8fa "hw/rdma: Switch to generic error reporting way"
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/rdma/vmw/pvrdma_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 3e36e130139c..d370ae07ca6a 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -667,7 +667,7 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
out:
if (rc) {
pvrdma_fini(pdev);
- error_append_hint(errp, "Device failed to load\n");
+ rdma_error_report("Device failed to load");
}
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report()
2019-09-17 10:21 ` [Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report() Greg Kurz
@ 2019-09-17 14:51 ` Yuval Shaia
2019-09-17 16:15 ` Greg Kurz
0 siblings, 1 reply; 49+ messages in thread
From: Yuval Shaia @ 2019-09-17 14:51 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Max Reitz, Juan Quintela, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Subbaraya Sundeep, Paolo Bonzini
On Tue, Sep 17, 2019 at 12:21:27PM +0200, Greg Kurz wrote:
> Commit 4d71b38ae8fa converted many error_setg() call sites to
> rdma_error_report(), but it forgot to convert a companion
> error_append_hint(). Since no guy doesn't set errp anymore in
> pvrdma_realize(), errp remains NULL and error_append_hint() does
> nothing.
>
> Also error_append_hint() was a poor choice since its "intended use
> is adding helpful hints on the human user interface" and "not for
> clarifying a confusing error message".
>
> Call rdma_error_report() instead.
Thanks,
So are you suggesting to replace all other error_setg calls with
rdma_error_report instead?
>
> Fixes: 4d71b38ae8fa "hw/rdma: Switch to generic error reporting way"
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/rdma/vmw/pvrdma_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 3e36e130139c..d370ae07ca6a 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -667,7 +667,7 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
> out:
> if (rc) {
> pvrdma_fini(pdev);
> - error_append_hint(errp, "Device failed to load\n");
> + rdma_error_report("Device failed to load");
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> }
> }
>
>
>
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report()
2019-09-17 14:51 ` Yuval Shaia
@ 2019-09-17 16:15 ` Greg Kurz
0 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 16:15 UTC (permalink / raw)
To: Yuval Shaia
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Max Reitz, Juan Quintela, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Subbaraya Sundeep, Paolo Bonzini
On Tue, 17 Sep 2019 17:51:57 +0300
Yuval Shaia <yuval.shaia@oracle.com> wrote:
> On Tue, Sep 17, 2019 at 12:21:27PM +0200, Greg Kurz wrote:
> > Commit 4d71b38ae8fa converted many error_setg() call sites to
> > rdma_error_report(), but it forgot to convert a companion
> > error_append_hint(). Since no guy doesn't set errp anymore in
> > pvrdma_realize(), errp remains NULL and error_append_hint() does
> > nothing.
> >
> > Also error_append_hint() was a poor choice since its "intended use
> > is adding helpful hints on the human user interface" and "not for
> > clarifying a confusing error message".
> >
> > Call rdma_error_report() instead.
>
> Thanks,
> So are you suggesting to replace all other error_setg calls with
> rdma_error_report instead?
>
No. I don't know what was the motivation behind 4d71b38ae8fa, I'm
just fixing what seems to be a leftover, which should have been
error_prepend() instead of error_append_hint() actually.
> >
> > Fixes: 4d71b38ae8fa "hw/rdma: Switch to generic error reporting way"
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/rdma/vmw/pvrdma_main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> > index 3e36e130139c..d370ae07ca6a 100644
> > --- a/hw/rdma/vmw/pvrdma_main.c
> > +++ b/hw/rdma/vmw/pvrdma_main.c
> > @@ -667,7 +667,7 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
> > out:
> > if (rc) {
> > pvrdma_fini(pdev);
> > - error_append_hint(errp, "Device failed to load\n");
> > + rdma_error_report("Device failed to load");
>
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
>
> > }
> > }
> >
> >
> >
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (8 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report() Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 11:24 ` Cornelia Huck
2019-09-17 10:21 ` [Qemu-devel] [PATCH 11/17] scsi: " Greg Kurz
` (7 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, Mark Cave-Ayland, Michael Roth,
Gerd Hoffmann, Subbaraya Sundeep, Juan Quintela,
David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/s390x/s390-ccw.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 0c5a5b60bd6b..9566be800a3b 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -57,11 +57,13 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
{
unsigned int cssid, ssid, devid;
char dev_path[PATH_MAX] = {0}, *tmp;
+ Error *err = NULL;
if (!sysfsdev) {
- error_setg(errp, "No host device provided");
- error_append_hint(errp,
+ error_setg(&err, "No host device provided");
+ error_append_hint(&err,
"Use -device vfio-ccw,sysfsdev=PATH_TO_DEVICE\n");
+ error_propagate(errp, err);
return;
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()
2019-09-17 10:21 ` [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint() Greg Kurz
@ 2019-09-17 11:24 ` Cornelia Huck
2019-09-17 11:44 ` David Hildenbrand
2019-09-17 16:36 ` Greg Kurz
0 siblings, 2 replies; 49+ messages in thread
From: Cornelia Huck @ 2019-09-17 11:24 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 12:21:34 +0200
Greg Kurz <groug@kaod.org> wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/s390x/s390-ccw.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Can also take this via the s390 tree, let me know what would work best.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()
2019-09-17 11:24 ` Cornelia Huck
@ 2019-09-17 11:44 ` David Hildenbrand
2019-09-17 16:36 ` Greg Kurz
1 sibling, 0 replies; 49+ messages in thread
From: David Hildenbrand @ 2019-09-17 11:44 UTC (permalink / raw)
To: Cornelia Huck, Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, qemu-arm, John Snow, Richard Henderson,
Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On 17.09.19 13:24, Cornelia Huck wrote:
> On Tue, 17 Sep 2019 12:21:34 +0200
> Greg Kurz <groug@kaod.org> wrote:
>
>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>
>> Signed-off-by: Greg Kurz <groug@kaod.org>
>> ---
>> hw/s390x/s390-ccw.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> Can also take this via the s390 tree, let me know what would work best.
>
Thanks for fixing the cc list :)
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()
2019-09-17 11:24 ` Cornelia Huck
2019-09-17 11:44 ` David Hildenbrand
@ 2019-09-17 16:36 ` Greg Kurz
2019-09-18 10:26 ` Cornelia Huck
1 sibling, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 16:36 UTC (permalink / raw)
To: Cornelia Huck
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 13:24:12 +0200
Cornelia Huck <cohuck@redhat.com> wrote:
> On Tue, 17 Sep 2019 12:21:34 +0200
> Greg Kurz <groug@kaod.org> wrote:
>
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/s390x/s390-ccw.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> Can also take this via the s390 tree, let me know what would work best.
I guess it would be easier to merge if each individual patch goes
through the corresponding sub-maintainer tree. But Eric mentioned
in another mail that the whole whole series could maybe go through
Markus' error tree... so, I don't know which is best. :)
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()
2019-09-17 16:36 ` Greg Kurz
@ 2019-09-18 10:26 ` Cornelia Huck
2019-09-18 17:46 ` Eric Blake
0 siblings, 1 reply; 49+ messages in thread
From: Cornelia Huck @ 2019-09-18 10:26 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 18:36:20 +0200
Greg Kurz <groug@kaod.org> wrote:
> On Tue, 17 Sep 2019 13:24:12 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
> > On Tue, 17 Sep 2019 12:21:34 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> >
> > > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > hw/s390x/s390-ccw.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >
> > Can also take this via the s390 tree, let me know what would work best.
>
> I guess it would be easier to merge if each individual patch goes
> through the corresponding sub-maintainer tree. But Eric mentioned
> in another mail that the whole whole series could maybe go through
> Markus' error tree... so, I don't know which is best. :)
Ok, it's probably best to take this through the s390 tree, as I plan to
send a pull request tomorrow :)
Thanks, applied.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()
2019-09-18 10:26 ` Cornelia Huck
@ 2019-09-18 17:46 ` Eric Blake
2019-09-19 8:50 ` Cornelia Huck
0 siblings, 1 reply; 49+ messages in thread
From: Eric Blake @ 2019-09-18 17:46 UTC (permalink / raw)
To: Cornelia Huck, Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On 9/18/19 5:26 AM, Cornelia Huck wrote:
> On Tue, 17 Sep 2019 18:36:20 +0200
> Greg Kurz <groug@kaod.org> wrote:
>
>> On Tue, 17 Sep 2019 13:24:12 +0200
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> On Tue, 17 Sep 2019 12:21:34 +0200
>>> Greg Kurz <groug@kaod.org> wrote:
>>>
>>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>>
>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>> ---
>>>> hw/s390x/s390-ccw.c | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>
>>> Can also take this via the s390 tree, let me know what would work best.
>>
>> I guess it would be easier to merge if each individual patch goes
>> through the corresponding sub-maintainer tree. But Eric mentioned
>> in another mail that the whole whole series could maybe go through
>> Markus' error tree... so, I don't know which is best. :)
>
> Ok, it's probably best to take this through the s390 tree, as I plan to
> send a pull request tomorrow :)
If we go with Vladimir's idea of auto-propagation, this change just gets
rewritten again as part of our simplifications to drop all use of
'local_err' in favor of instead using the macro that makes errp always
safe to use locally.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()
2019-09-18 17:46 ` Eric Blake
@ 2019-09-19 8:50 ` Cornelia Huck
0 siblings, 0 replies; 49+ messages in thread
From: Cornelia Huck @ 2019-09-19 8:50 UTC (permalink / raw)
To: Eric Blake
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman, Greg Kurz,
Yuval Shaia, Dr. David Alan Gilbert, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Wed, 18 Sep 2019 12:46:42 -0500
Eric Blake <eblake@redhat.com> wrote:
> On 9/18/19 5:26 AM, Cornelia Huck wrote:
> > On Tue, 17 Sep 2019 18:36:20 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> >
> >> On Tue, 17 Sep 2019 13:24:12 +0200
> >> Cornelia Huck <cohuck@redhat.com> wrote:
> >>
> >>> On Tue, 17 Sep 2019 12:21:34 +0200
> >>> Greg Kurz <groug@kaod.org> wrote:
> >>>
> >>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >>>>
> >>>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>>> ---
> >>>> hw/s390x/s390-ccw.c | 6 ++++--
> >>>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >>>
> >>> Can also take this via the s390 tree, let me know what would work best.
> >>
> >> I guess it would be easier to merge if each individual patch goes
> >> through the corresponding sub-maintainer tree. But Eric mentioned
> >> in another mail that the whole whole series could maybe go through
> >> Markus' error tree... so, I don't know which is best. :)
> >
> > Ok, it's probably best to take this through the s390 tree, as I plan to
> > send a pull request tomorrow :)
>
> If we go with Vladimir's idea of auto-propagation, this change just gets
> rewritten again as part of our simplifications to drop all use of
> 'local_err' in favor of instead using the macro that makes errp always
> safe to use locally.
>
Fair enough. That auto-propagation approach really looks nice, so I'll
go ahead and unqueue this patch again.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Qemu-devel] [PATCH 11/17] scsi: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (9 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint() Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 10:21 ` [Qemu-devel] [PATCH 12/17] migration: " Greg Kurz
` (6 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, Eric Farman, Dr. David Alan Gilbert,
Yuval Shaia, Alex Williamson, John Snow, Richard Henderson,
Kevin Wolf, Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/scsi/scsi-disk.c | 7 +++++--
hw/scsi/scsi-generic.c | 7 +++++--
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 915641a0f1b4..3d5e257bb66b 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2619,9 +2619,12 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
/* check we are using a driver managing SG_IO (version 3 and after) */
rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
if (rc < 0) {
- error_setg_errno(errp, -rc, "cannot get SG_IO version number");
+ Error *local_err = NULL;
+
+ error_setg_errno(&local_err, -rc, "cannot get SG_IO version number");
if (rc != -EPERM) {
- error_append_hint(errp, "Is this a SCSI device?\n");
+ error_append_hint(&local_err, "Is this a SCSI device?\n");
+ error_propagate(errp, local_err);
}
goto out;
}
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index e7798ebcd0d4..f6a5b538cbcc 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -674,9 +674,12 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
/* check we are using a driver managing SG_IO (version 3 and after */
rc = blk_ioctl(s->conf.blk, SG_GET_VERSION_NUM, &sg_version);
if (rc < 0) {
- error_setg_errno(errp, -rc, "cannot get SG_IO version number");
+ Error *local_err = NULL;
+
+ error_setg_errno(&local_err, -rc, "cannot get SG_IO version number");
if (rc != -EPERM) {
- error_append_hint(errp, "Is this a SCSI device?\n");
+ error_append_hint(&local_err, "Is this a SCSI device?\n");
+ error_propagate(errp, local_err);
}
return;
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Qemu-devel] [PATCH 12/17] migration: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (10 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 11/17] scsi: " Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 10:32 ` Dr. David Alan Gilbert
2019-09-17 10:21 ` [Qemu-devel] [PATCH 13/17] nbd: " Greg Kurz
` (5 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
migration/migration.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 01863a95f5fe..6724173ce34e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -983,18 +983,24 @@ static bool migrate_caps_check(bool *cap_list,
#ifndef CONFIG_LIVE_BLOCK_MIGRATION
if (cap_list[MIGRATION_CAPABILITY_BLOCK]) {
- error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "QEMU compiled without old-style (blk/-b, inc/-i) "
"block migration");
- error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+ error_append_hint(&local_err, "Use drive_mirror+NBD instead.\n");
+ error_propagate(errp, local_err);
return false;
}
#endif
#ifndef CONFIG_REPLICATION
if (cap_list[MIGRATION_CAPABILITY_X_COLO]) {
- error_setg(errp, "QEMU compiled without replication module"
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "QEMU compiled without replication module"
" can't enable COLO");
- error_append_hint(errp, "Please enable replication before COLO.\n");
+ error_append_hint(&local_err, "Please enable replication before COLO.\n");
+ error_propagate(errp, local_err);
return false;
}
#endif
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 12/17] migration: Pass local error object pointer to error_append_hint()
2019-09-17 10:21 ` [Qemu-devel] [PATCH 12/17] migration: " Greg Kurz
@ 2019-09-17 10:32 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 49+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-17 10:32 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
* Greg Kurz (groug@kaod.org) wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
A bit painful, but if it solves the problem,
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/migration.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 01863a95f5fe..6724173ce34e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -983,18 +983,24 @@ static bool migrate_caps_check(bool *cap_list,
>
> #ifndef CONFIG_LIVE_BLOCK_MIGRATION
> if (cap_list[MIGRATION_CAPABILITY_BLOCK]) {
> - error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
> + Error *local_err = NULL;
> +
> + error_setg(&local_err, "QEMU compiled without old-style (blk/-b, inc/-i) "
> "block migration");
> - error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
> + error_append_hint(&local_err, "Use drive_mirror+NBD instead.\n");
> + error_propagate(errp, local_err);
> return false;
> }
> #endif
>
> #ifndef CONFIG_REPLICATION
> if (cap_list[MIGRATION_CAPABILITY_X_COLO]) {
> - error_setg(errp, "QEMU compiled without replication module"
> + Error *local_err = NULL;
> +
> + error_setg(&local_err, "QEMU compiled without replication module"
> " can't enable COLO");
> - error_append_hint(errp, "Please enable replication before COLO.\n");
> + error_append_hint(&local_err, "Please enable replication before COLO.\n");
> + error_propagate(errp, local_err);
> return false;
> }
> #endif
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Qemu-devel] [PATCH 13/17] nbd: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (11 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 12/17] migration: " Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 15:15 ` Eric Blake
2019-09-17 10:21 ` [Qemu-devel] [PATCH 14/17] ccid-card-emul: " Greg Kurz
` (4 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
nbd/client.c | 24 +++++++++++++-----------
nbd/server.c | 7 +++++--
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index b9dc829175f9..c6e6e4046fd5 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -154,6 +154,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
bool strict, Error **errp)
{
g_autofree char *msg = NULL;
+ Error *local_err = NULL;
if (!(reply->type & (1 << 31))) {
return 1;
@@ -161,14 +162,14 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
if (reply->length) {
if (reply->length > NBD_MAX_BUFFER_SIZE) {
- error_setg(errp, "server error %" PRIu32
+ error_setg(&local_err, "server error %" PRIu32
" (%s) message is too long",
reply->type, nbd_rep_lookup(reply->type));
goto err;
}
msg = g_malloc(reply->length + 1);
if (nbd_read(ioc, msg, reply->length, NULL, errp) < 0) {
- error_prepend(errp, "Failed to read option error %" PRIu32
+ error_prepend(&local_err, "Failed to read option error %" PRIu32
" (%s) message: ",
reply->type, nbd_rep_lookup(reply->type));
goto err;
@@ -187,50 +188,51 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
switch (reply->type) {
case NBD_REP_ERR_POLICY:
- error_setg(errp, "Denied by server for option %" PRIu32 " (%s)",
+ error_setg(&local_err, "Denied by server for option %" PRIu32 " (%s)",
reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_INVALID:
- error_setg(errp, "Invalid parameters for option %" PRIu32 " (%s)",
+ error_setg(&local_err, "Invalid parameters for option %" PRIu32 " (%s)",
reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_PLATFORM:
- error_setg(errp, "Server lacks support for option %" PRIu32 " (%s)",
+ error_setg(&local_err, "Server lacks support for option %" PRIu32 " (%s)",
reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_TLS_REQD:
- error_setg(errp, "TLS negotiation required before option %" PRIu32
+ error_setg(&local_err, "TLS negotiation required before option %" PRIu32
" (%s)", reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_UNKNOWN:
- error_setg(errp, "Requested export not available");
+ error_setg(&local_err, "Requested export not available");
break;
case NBD_REP_ERR_SHUTDOWN:
- error_setg(errp, "Server shutting down before option %" PRIu32 " (%s)",
+ error_setg(&local_err, "Server shutting down before option %" PRIu32 " (%s)",
reply->option, nbd_opt_lookup(reply->option));
break;
case NBD_REP_ERR_BLOCK_SIZE_REQD:
- error_setg(errp, "Server requires INFO_BLOCK_SIZE for option %" PRIu32
+ error_setg(&local_err, "Server requires INFO_BLOCK_SIZE for option %" PRIu32
" (%s)", reply->option, nbd_opt_lookup(reply->option));
break;
default:
- error_setg(errp, "Unknown error code when asking for option %" PRIu32
+ error_setg(&local_err, "Unknown error code when asking for option %" PRIu32
" (%s)", reply->option, nbd_opt_lookup(reply->option));
break;
}
if (msg) {
- error_append_hint(errp, "server reported: %s\n", msg);
+ error_append_hint(&local_err, "server reported: %s\n", msg);
}
err:
+ error_propagate(errp, local_err);
nbd_send_opt_abort(ioc);
return -1;
}
diff --git a/nbd/server.c b/nbd/server.c
index 28c3c8be854c..6d9ca2563cce 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1616,6 +1616,8 @@ void nbd_export_close(NBDExport *exp)
void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
{
+ Error *local_err = NULL;
+
if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
nbd_export_close(exp);
return;
@@ -1623,8 +1625,9 @@ void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
assert(mode == NBD_SERVER_REMOVE_MODE_SAFE);
- error_setg(errp, "export '%s' still in use", exp->name);
- error_append_hint(errp, "Use mode='hard' to force client disconnect\n");
+ error_setg(&local_err, "export '%s' still in use", exp->name);
+ error_append_hint(&local_err, "Use mode='hard' to force client disconnect\n");
+ error_propagate(errp, local_err);
}
void nbd_export_get(NBDExport *exp)
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 13/17] nbd: Pass local error object pointer to error_append_hint()
2019-09-17 10:21 ` [Qemu-devel] [PATCH 13/17] nbd: " Greg Kurz
@ 2019-09-17 15:15 ` Eric Blake
2019-09-17 16:26 ` Greg Kurz
0 siblings, 1 reply; 49+ messages in thread
From: Eric Blake @ 2019-09-17 15:15 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
qemu-block, Juan Quintela, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, qemu-arm, John Snow, Richard Henderson,
Kevin Wolf, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On 9/17/19 5:21 AM, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> nbd/client.c | 24 +++++++++++++-----------
> nbd/server.c | 7 +++++--
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index b9dc829175f9..c6e6e4046fd5 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -154,6 +154,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> bool strict, Error **errp)
> {
> g_autofree char *msg = NULL;
> + Error *local_err = NULL;
I'd prefer 'err' here...
>
> if (!(reply->type & (1 << 31))) {
> return 1;
> @@ -161,14 +162,14 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
>
> if (reply->length) {
> if (reply->length > NBD_MAX_BUFFER_SIZE) {
> - error_setg(errp, "server error %" PRIu32
> + error_setg(&local_err, "server error %" PRIu32
so that &err doesn't change line lengths.
> case NBD_REP_ERR_SHUTDOWN:
> - error_setg(errp, "Server shutting down before option %" PRIu32 " (%s)",
> + error_setg(&local_err, "Server shutting down before option %" PRIu32 " (%s)",
For example, here, you went beyond 80 columns.
At any rate, I'm assuming this will probably go in through Markus' error
tree as part of the whole series, rather than me needing to pick just
this patch through my NBD tree.
Whether or not you shorten the local variable name,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 13/17] nbd: Pass local error object pointer to error_append_hint()
2019-09-17 15:15 ` Eric Blake
@ 2019-09-17 16:26 ` Greg Kurz
0 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 16:26 UTC (permalink / raw)
To: Eric Blake
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 10:15:07 -0500
Eric Blake <eblake@redhat.com> wrote:
> On 9/17/19 5:21 AM, Greg Kurz wrote:
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > nbd/client.c | 24 +++++++++++++-----------
> > nbd/server.c | 7 +++++--
> > 2 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/nbd/client.c b/nbd/client.c
> > index b9dc829175f9..c6e6e4046fd5 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -154,6 +154,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> > bool strict, Error **errp)
> > {
> > g_autofree char *msg = NULL;
> > + Error *local_err = NULL;
>
> I'd prefer 'err' here...
>
> >
> > if (!(reply->type & (1 << 31))) {
> > return 1;
> > @@ -161,14 +162,14 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> >
> > if (reply->length) {
> > if (reply->length > NBD_MAX_BUFFER_SIZE) {
> > - error_setg(errp, "server error %" PRIu32
> > + error_setg(&local_err, "server error %" PRIu32
>
> so that &err doesn't change line lengths.
>
> > case NBD_REP_ERR_SHUTDOWN:
> > - error_setg(errp, "Server shutting down before option %" PRIu32 " (%s)",
> > + error_setg(&local_err, "Server shutting down before option %" PRIu32 " (%s)",
>
> For example, here, you went beyond 80 columns.
>
> At any rate, I'm assuming this will probably go in through Markus' error
> tree as part of the whole series, rather than me needing to pick just
> this patch through my NBD tree.
>
> Whether or not you shorten the local variable name,
>
Regardless of which tree this goes through, it will be code for
which you're the official maintainer in the end. I'll gladly fix
it to meet your needs :)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Qemu-devel] [PATCH 14/17] ccid-card-emul: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (12 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 13/17] nbd: " Greg Kurz
@ 2019-09-17 10:21 ` Greg Kurz
2019-09-17 10:22 ` [Qemu-devel] [PATCH 15/17] option: " Greg Kurz
` (3 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, John Snow,
Richard Henderson, Kevin Wolf, Cornelia Huck, Max Reitz,
Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/usb/ccid-card-emulated.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 291e41db8a1e..3bc397341090 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -511,10 +511,13 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
}
if (card->backend == 0) {
- error_setg(errp, "backend must be one of:");
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "backend must be one of:");
for (ptable = backend_enum_table; ptable->name != NULL; ++ptable) {
- error_append_hint(errp, "%s\n", ptable->name);
+ error_append_hint(&local_err, "%s\n", ptable->name);
}
+ error_propagate(errp, local_err);
goto out2;
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Qemu-devel] [PATCH 15/17] option: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (13 preceding siblings ...)
2019-09-17 10:21 ` [Qemu-devel] [PATCH 14/17] ccid-card-emul: " Greg Kurz
@ 2019-09-17 10:22 ` Greg Kurz
2019-09-17 10:22 ` [Qemu-devel] [PATCH 16/17] socket: " Greg Kurz
` (2 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:22 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
util/qemu-option.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 97172b5eaa7f..2a45dfa585d4 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -155,11 +155,14 @@ void parse_option_size(const char *name, const char *value,
return;
}
if (err) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
+ Error *local_err = NULL;
+
+ error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, name,
"a non-negative number below 2^64");
- error_append_hint(errp, "Optional suffix k, M, G, T, P or E means"
+ error_append_hint(&local_err, "Optional suffix k, M, G, T, P or E means"
" kilo-, mega-, giga-, tera-, peta-\n"
"and exabytes, respectively.\n");
+ error_propagate(errp, local_err);
return;
}
*ret = size;
@@ -664,10 +667,13 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
if (id) {
if (!id_wellformed(id)) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
+ Error *local_err = NULL;
+
+ error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "id",
"an identifier");
- error_append_hint(errp, "Identifiers consist of letters, digits, "
+ error_append_hint(&local_err, "Identifiers consist of letters, digits, "
"'-', '.', '_', starting with a letter.\n");
+ error_propagate(errp, local_err);
return NULL;
}
opts = qemu_opts_find(list, id);
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Qemu-devel] [PATCH 16/17] socket: Pass local error object pointer to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (14 preceding siblings ...)
2019-09-17 10:22 ` [Qemu-devel] [PATCH 15/17] option: " Greg Kurz
@ 2019-09-17 10:22 ` Greg Kurz
2019-09-17 10:22 ` [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed " Greg Kurz
2019-09-17 11:00 ` [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Philippe Mathieu-Daudé
17 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:22 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Subbaraya Sundeep,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
util/qemu-sockets.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 98ff3a1cce64..5da7b5f2fa0d 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -882,9 +882,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
pathlen = strlen(path);
if (pathlen > sizeof(un.sun_path)) {
- error_setg(errp, "UNIX socket path '%s' is too long", path);
- error_append_hint(errp, "Path must be less than %zu bytes\n",
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "UNIX socket path '%s' is too long", path);
+ error_append_hint(&local_err, "Path must be less than %zu bytes\n",
sizeof(un.sun_path));
+ error_propagate(errp, local_err);
goto err;
}
@@ -952,9 +955,12 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
pathlen = strlen(saddr->path);
if (pathlen > sizeof(un.sun_path)) {
- error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
- error_append_hint(errp, "Path must be less than %zu bytes\n",
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "UNIX socket path '%s' is too long", saddr->path);
+ error_append_hint(&local_err, "Path must be less than %zu bytes\n",
sizeof(un.sun_path));
+ error_propagate(errp, local_err);
goto err;
}
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (15 preceding siblings ...)
2019-09-17 10:22 ` [Qemu-devel] [PATCH 16/17] socket: " Greg Kurz
@ 2019-09-17 10:22 ` Greg Kurz
2019-09-17 10:56 ` Philippe Mathieu-Daudé
2019-09-17 11:29 ` Cornelia Huck
2019-09-17 11:00 ` [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Philippe Mathieu-Daudé
17 siblings, 2 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 10:22 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Daniel P. =?utf-8?q?Berrang=C3=A9=22?=
<berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org, Mark Cave-Ayland, Michael Roth,
Gerd Hoffmann, Subbaraya Sundeep, Juan Quintela,
David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Cornelia Huck, Max Reitz, Paolo Bonzini
Passing errp from the argument list to error_append_hint()
isn't recommended because it may cause unwanted behaviours
when errp is equal to &error_fatal or &error_abort.
First, error_append_hint() aborts QEMU when passed either of
those.
Second, consider the following:
void foo(Error **errp)
{
error_setg(errp, "foo");
error_append_hint(errp, "Try bar\n");
}
error_setg() causes QEMU to exit or abort, and hints aren't
added.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
scripts/checkpatch.pl | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index aa9a354a0e7e..17ce026282a6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2902,6 +2902,10 @@ sub process {
}
}
+ if ($line =~ /error_append_hint\(errp/) {
+ WARN("suspicious errp passed to error_append_hint()\n" .
+ $herecurr);
+ }
# check for non-portable libc calls that have portable alternatives in QEMU
if ($line =~ /\bffs\(/) {
ERROR("use ctz32() instead of ffs()\n" . $herecurr);
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()
2019-09-17 10:22 ` [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed " Greg Kurz
@ 2019-09-17 10:56 ` Philippe Mathieu-Daudé
2019-09-17 11:29 ` Cornelia Huck
1 sibling, 0 replies; 49+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-17 10:56 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Max Reitz,
Juan Quintela, David Hildenbrand, Markus Armbruster, Halil Pasic,
Christian Borntraeger, Marc-André Lureau, David Gibson,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, John Snow, Richard Henderson, Kevin Wolf,
Daniel P. Berrangé,
Cornelia Huck, Subbaraya Sundeep, Paolo Bonzini
On 9/17/19 12:22 PM, Greg Kurz wrote:
> Passing errp from the argument list to error_append_hint()
> isn't recommended because it may cause unwanted behaviours
> when errp is equal to &error_fatal or &error_abort.
>
> First, error_append_hint() aborts QEMU when passed either of
> those.
>
> Second, consider the following:
>
> void foo(Error **errp)
> {
> error_setg(errp, "foo");
> error_append_hint(errp, "Try bar\n");
> }
>
> error_setg() causes QEMU to exit or abort, and hints aren't
> added.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> scripts/checkpatch.pl | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index aa9a354a0e7e..17ce026282a6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2902,6 +2902,10 @@ sub process {
> }
> }
>
> + if ($line =~ /error_append_hint\(errp/) {
Checking for 'errp' variable name seems fragile, but all the codebase
uses exactly this name, so it is sufficient.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> + WARN("suspicious errp passed to error_append_hint()\n" .
> + $herecurr);
> + }
> # check for non-portable libc calls that have portable alternatives in QEMU
> if ($line =~ /\bffs\(/) {
> ERROR("use ctz32() instead of ffs()\n" . $herecurr);
>
>
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()
2019-09-17 10:22 ` [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed " Greg Kurz
2019-09-17 10:56 ` Philippe Mathieu-Daudé
@ 2019-09-17 11:29 ` Cornelia Huck
2019-09-17 11:47 ` Greg Kurz
1 sibling, 1 reply; 49+ messages in thread
From: Cornelia Huck @ 2019-09-17 11:29 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 12:22:18 +0200
Greg Kurz <groug@kaod.org> wrote:
> Passing errp from the argument list to error_append_hint()
> isn't recommended because it may cause unwanted behaviours
> when errp is equal to &error_fatal or &error_abort.
>
> First, error_append_hint() aborts QEMU when passed either of
> those.
>
> Second, consider the following:
>
> void foo(Error **errp)
> {
> error_setg(errp, "foo");
> error_append_hint(errp, "Try bar\n");
> }
>
> error_setg() causes QEMU to exit or abort, and hints aren't
> added.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> scripts/checkpatch.pl | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index aa9a354a0e7e..17ce026282a6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2902,6 +2902,10 @@ sub process {
> }
> }
>
Maybe add a comment here?
# using errp is common practice, so that check should hopefully be enough
> + if ($line =~ /error_append_hint\(errp/) {
> + WARN("suspicious errp passed to error_append_hint()\n" .
Add "(use a local error object)"?
> + $herecurr);
> + }
> # check for non-portable libc calls that have portable alternatives in QEMU
> if ($line =~ /\bffs\(/) {
> ERROR("use ctz32() instead of ffs()\n" . $herecurr);
>
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()
2019-09-17 11:29 ` Cornelia Huck
@ 2019-09-17 11:47 ` Greg Kurz
0 siblings, 0 replies; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 11:47 UTC (permalink / raw)
To: Cornelia Huck
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Subbaraya Sundeep, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, David Gibson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, Richard Henderson, Kevin Wolf, Daniel P. Berrangé,
qemu-s390x, Max Reitz, qemu-ppc, Paolo Bonzini
On Tue, 17 Sep 2019 13:29:36 +0200
Cornelia Huck <cohuck@redhat.com> wrote:
> On Tue, 17 Sep 2019 12:22:18 +0200
> Greg Kurz <groug@kaod.org> wrote:
>
> > Passing errp from the argument list to error_append_hint()
> > isn't recommended because it may cause unwanted behaviours
> > when errp is equal to &error_fatal or &error_abort.
> >
> > First, error_append_hint() aborts QEMU when passed either of
> > those.
> >
> > Second, consider the following:
> >
> > void foo(Error **errp)
> > {
> > error_setg(errp, "foo");
> > error_append_hint(errp, "Try bar\n");
> > }
> >
> > error_setg() causes QEMU to exit or abort, and hints aren't
> > added.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > scripts/checkpatch.pl | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index aa9a354a0e7e..17ce026282a6 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2902,6 +2902,10 @@ sub process {
> > }
> > }
> >
>
> Maybe add a comment here?
>
> # using errp is common practice, so that check should hopefully be enough
>
> > + if ($line =~ /error_append_hint\(errp/) {
> > + WARN("suspicious errp passed to error_append_hint()\n" .
>
> Add "(use a local error object)"?
>
Sure. I'll add both.
> > + $herecurr);
> > + }
> > # check for non-portable libc calls that have portable alternatives in QEMU
> > if ($line =~ /\bffs\(/) {
> > ERROR("use ctz32() instead of ffs()\n" . $herecurr);
> >
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint()
2019-09-17 10:20 [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Greg Kurz
` (16 preceding siblings ...)
2019-09-17 10:22 ` [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed " Greg Kurz
@ 2019-09-17 11:00 ` Philippe Mathieu-Daudé
2019-09-17 11:45 ` Greg Kurz
17 siblings, 1 reply; 49+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-09-17 11:00 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, Michael Roth, Gerd Hoffmann, Max Reitz,
qemu-block, Juan Quintela, David Hildenbrand, Markus Armbruster,
Halil Pasic, Christian Borntraeger, Marc-André Lureau,
David Gibson, Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, qemu-arm, John Snow, Richard Henderson,
Kevin Wolf, Daniel P. Berrangé,
Cornelia Huck, Subbaraya Sundeep, qemu-ppc, Paolo Bonzini
For some reason your email client escaped incorrectly Daniel's email:
"Daniel P. Berrangé\" <berrange@redhat.com>
Which makes my email client very unhappy (Thunderbird):
There are non-ASCII characters in the local part of the recipient
address "Daniel P. Berrangé <berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
supported. Please change this address and try again.
Neither is MTA:
An error occurred while sending mail. The mail server responded:
5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
Please check the message recipient
""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint()
2019-09-17 11:00 ` [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint() Philippe Mathieu-Daudé
@ 2019-09-17 11:45 ` Greg Kurz
2019-09-17 11:49 ` Daniel P. Berrangé
0 siblings, 1 reply; 49+ messages in thread
From: Greg Kurz @ 2019-09-17 11:45 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Max Reitz, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, Richard Henderson, Eric Farman,
Dr. David Alan Gilbert, Yuval Shaia, Alex Williamson, qemu-arm,
John Snow, David Gibson, Kevin Wolf, Daniel P. Berrangé,
Cornelia Huck, qemu-s390x, Subbaraya Sundeep, qemu-ppc,
Paolo Bonzini
On Tue, 17 Sep 2019 13:00:37 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> For some reason your email client escaped incorrectly Daniel's email:
>
> "Daniel P. Berrangé\" <berrange@redhat.com>
>
The client is stgit's "stg mail" command, and indeed it generates this:
"Daniel P. =?utf-8?q?Berrang=C3=A9=22?= <berrange@redhat.com>
^^^
double-quote
and if I turn the 'é' into a 'e':
"Daniel P. Berrange" <berrange@redhat.com>
This looks like a bug in "stg mail"...
> Which makes my email client very unhappy (Thunderbird):
>
> There are non-ASCII characters in the local part of the recipient
> address "Daniel P. Berrangé <berrange@redhat.com>,
> qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
> qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
> supported. Please change this address and try again.
>
> Neither is MTA:
>
> An error occurred while sending mail. The mail server responded:
> 5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
> 5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
> Please check the message recipient
> ""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.
Drat... this may prevent recipients starting with Daniel from receiving
the mails... ie, the sub-lists IIUC. Maybe not worth reposting for that.
Hopefully, Daniel may catch up on qemu-devel.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint()
2019-09-17 11:45 ` Greg Kurz
@ 2019-09-17 11:49 ` Daniel P. Berrangé
0 siblings, 0 replies; 49+ messages in thread
From: Daniel P. Berrangé @ 2019-09-17 11:49 UTC (permalink / raw)
To: Greg Kurz
Cc: Fam Zheng, Peter Maydell, Michael S. Tsirkin, Jeff Cody,
Mark Cave-Ayland, qemu-devel, Michael Roth, Gerd Hoffmann,
Max Reitz, qemu-block, Juan Quintela, David Hildenbrand,
Markus Armbruster, Halil Pasic, Christian Borntraeger,
Marc-André Lureau, Richard Henderson,
Philippe Mathieu-Daudé,
Eric Farman, Dr. David Alan Gilbert, Yuval Shaia,
Alex Williamson, qemu-arm, John Snow, David Gibson, Kevin Wolf,
Cornelia Huck, qemu-s390x, Subbaraya Sundeep, qemu-ppc,
Paolo Bonzini
On Tue, Sep 17, 2019 at 01:45:29PM +0200, Greg Kurz wrote:
> On Tue, 17 Sep 2019 13:00:37 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> > For some reason your email client escaped incorrectly Daniel's email:
> >
> > "Daniel P. Berrangé\" <berrange@redhat.com>
> >
>
> The client is stgit's "stg mail" command, and indeed it generates this:
>
> "Daniel P. =?utf-8?q?Berrang=C3=A9=22?= <berrange@redhat.com>
> ^^^
> double-quote
>
> and if I turn the 'é' into a 'e':
>
> "Daniel P. Berrange" <berrange@redhat.com>
>
> This looks like a bug in "stg mail"...
Happy to see that my name is detecting bugs :-)
I should add an emoji to it to really flush out the edge cases...
> > Which makes my email client very unhappy (Thunderbird):
> >
> > There are non-ASCII characters in the local part of the recipient
> > address "Daniel P. Berrangé <berrange@redhat.com>,
> > qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
> > qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
> > supported. Please change this address and try again.
> >
> > Neither is MTA:
> >
> > An error occurred while sending mail. The mail server responded:
> > 5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
> > 5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
> > Please check the message recipient
> > ""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.
>
> Drat... this may prevent recipients starting with Daniel from receiving
> the mails... ie, the sub-lists IIUC. Maybe not worth reposting for that.
Yeah, I wouldn't bother reposting just for that.
> Hopefully, Daniel may catch up on qemu-devel.
Indeed.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 49+ messages in thread