* [Qemu-devel] [PATCH for-2.6 0/3] Fix dangling pointers and error message regressions @ 2016-04-27 14:29 Markus Armbruster 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 1/3] QemuOpts: Fix qemu_opts_foreach() dangling location regression Markus Armbruster ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Markus Armbruster @ 2016-04-27 14:29 UTC (permalink / raw) To: qemu-devel PATCH 1+2 are simple fixes for dangling pointers to unused stack, and as such belong into 2.6 if at all possible. PATCH 3 fixes an error message regression. The patch is a bit long, but repetitive. We may decide it's too late for 2.6 and route it to -stable instead. Markus Armbruster (3): QemuOpts: Fix qemu_opts_foreach() dangling location regression replay: Fix dangling location bug in replay_configure() qom: -object error messages lost location, restore it include/qom/object_interfaces.h | 5 +++-- qemu-img.c | 39 +++++++++++---------------------------- qemu-io.c | 3 +-- qemu-nbd.c | 3 +-- qom/object_interfaces.c | 4 +++- replay/replay.c | 3 ++- util/qemu-option.c | 6 +++--- vl.c | 6 ++---- 8 files changed, 26 insertions(+), 43 deletions(-) -- 2.5.5 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH for-2.6 1/3] QemuOpts: Fix qemu_opts_foreach() dangling location regression 2016-04-27 14:29 [Qemu-devel] [PATCH for-2.6 0/3] Fix dangling pointers and error message regressions Markus Armbruster @ 2016-04-27 14:29 ` Markus Armbruster 2016-04-27 14:42 ` Eric Blake 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 2/3] replay: Fix dangling location bug in replay_configure() Markus Armbruster 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it Markus Armbruster 2 siblings, 1 reply; 10+ messages in thread From: Markus Armbruster @ 2016-04-27 14:29 UTC (permalink / raw) To: qemu-devel qemu_opts_foreach() pushes and pops a Location with automatic storage duration. Except it fails to pop when @func() returns non-zero. cur_loc then points to unused stack space, and will most likely get clobbered in short order. Clobbered cur_loc can make loc_pop() and error_print_loc() crash or report bogus locations. Affects several qemu command line options as well as qemu-img, qemu-io, qemu-nbd -object, and blkdebug's configuration file. Broken in commit a4c7367, v2.4.0. Reproducer: $ qemu-system-x86_64 -nodefaults -display none -object secret,id=foo,foo=bar main() reports "Property '.foo' not found" like this: if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_add_opts_foreach, object_create_delayed, &err)) { error_report_err(err); exit(1); } cur_loc then points to where qemu_opts_foreach()'s Location used to be, i.e. unused stack space. With optimization, this Location doesn't get clobbered for me, and also happens to be the correct location. Without optimization, it does get clobbered in a way that makes error_report_err() report no location. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- util/qemu-option.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/util/qemu-option.c b/util/qemu-option.c index dd9e73d..3467dc2 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -1108,19 +1108,19 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, { Location loc; QemuOpts *opts; - int rc; + int rc = 0; loc_push_none(&loc); QTAILQ_FOREACH(opts, &list->head, next) { loc_restore(&opts->loc); rc = func(opaque, opts, errp); if (rc) { - return rc; + break; } assert(!errp || !*errp); } loc_pop(&loc); - return 0; + return rc; } static size_t count_opts_list(QemuOptsList *list) -- 2.5.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6 1/3] QemuOpts: Fix qemu_opts_foreach() dangling location regression 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 1/3] QemuOpts: Fix qemu_opts_foreach() dangling location regression Markus Armbruster @ 2016-04-27 14:42 ` Eric Blake 0 siblings, 0 replies; 10+ messages in thread From: Eric Blake @ 2016-04-27 14:42 UTC (permalink / raw) To: Markus Armbruster, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1488 bytes --] On 04/27/2016 08:29 AM, Markus Armbruster wrote: > qemu_opts_foreach() pushes and pops a Location with automatic storage > duration. Except it fails to pop when @func() returns non-zero. > cur_loc then points to unused stack space, and will most likely get > clobbered in short order. > > Clobbered cur_loc can make loc_pop() and error_print_loc() crash or > report bogus locations. > > Affects several qemu command line options as well as qemu-img, > qemu-io, qemu-nbd -object, and blkdebug's configuration file. > > Broken in commit a4c7367, v2.4.0. Latent bug means it's not a regression between 2.5 and 2.6, but I agree that if there is time to get this in 2.6, it is worth having. It's a shame that valgrind doesn't catch use of stale stack space. > cur_loc then points to where qemu_opts_foreach()'s Location used to > be, i.e. unused stack space. With optimization, this Location doesn't > get clobbered for me, and also happens to be the correct location. > Without optimization, it does get clobbered in a way that makes > error_report_err() report no location. And that explains why some people were having problems reproducing. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > util/qemu-option.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH for-2.6 2/3] replay: Fix dangling location bug in replay_configure() 2016-04-27 14:29 [Qemu-devel] [PATCH for-2.6 0/3] Fix dangling pointers and error message regressions Markus Armbruster 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 1/3] QemuOpts: Fix qemu_opts_foreach() dangling location regression Markus Armbruster @ 2016-04-27 14:29 ` Markus Armbruster 2016-04-27 14:57 ` Eric Blake 2016-04-27 16:39 ` Eduardo Habkost 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it Markus Armbruster 2 siblings, 2 replies; 10+ messages in thread From: Markus Armbruster @ 2016-04-27 14:29 UTC (permalink / raw) To: qemu-devel; +Cc: eblake, Eduardo Habkost replay_configure() pushes and pops a Location with automatic storage duration. Except it fails to pop when -icount parameter "rr" isn't given. cur_loc then points to unused stack space, and will most likely get clobbered in short order. Clobbered cur_loc can make loc_pop() and error_print_loc() crash or report bogus locations. Broken in commit 890ad55. I didn't take the time to find a reproducer. Cc: Eduardo Habkost <ehabkost@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- replay/replay.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/replay/replay.c b/replay/replay.c index 7c2573a..167fd29 100644 --- a/replay/replay.c +++ b/replay/replay.c @@ -275,7 +275,7 @@ void replay_configure(QemuOpts *opts) rr = qemu_opt_get(opts, "rr"); if (!rr) { /* Just enabling icount */ - return; + goto out; } else if (!strcmp(rr, "record")) { mode = REPLAY_MODE_RECORD; } else if (!strcmp(rr, "replay")) { @@ -293,6 +293,7 @@ void replay_configure(QemuOpts *opts) replay_enable(fname, mode); +out: loc_pop(&loc); } -- 2.5.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6 2/3] replay: Fix dangling location bug in replay_configure() 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 2/3] replay: Fix dangling location bug in replay_configure() Markus Armbruster @ 2016-04-27 14:57 ` Eric Blake 2016-04-27 16:39 ` Eduardo Habkost 1 sibling, 0 replies; 10+ messages in thread From: Eric Blake @ 2016-04-27 14:57 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: Eduardo Habkost [-- Attachment #1: Type: text/plain, Size: 841 bytes --] On 04/27/2016 08:29 AM, Markus Armbruster wrote: > replay_configure() pushes and pops a Location with automatic storage > duration. Except it fails to pop when -icount parameter "rr" isn't > given. cur_loc then points to unused stack space, and will most > likely get clobbered in short order. > > Clobbered cur_loc can make loc_pop() and error_print_loc() crash or > report bogus locations. > > Broken in commit 890ad55. > > I didn't take the time to find a reproducer. > > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > replay/replay.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6 2/3] replay: Fix dangling location bug in replay_configure() 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 2/3] replay: Fix dangling location bug in replay_configure() Markus Armbruster 2016-04-27 14:57 ` Eric Blake @ 2016-04-27 16:39 ` Eduardo Habkost 1 sibling, 0 replies; 10+ messages in thread From: Eduardo Habkost @ 2016-04-27 16:39 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, eblake On Wed, Apr 27, 2016 at 04:29:08PM +0200, Markus Armbruster wrote: > replay_configure() pushes and pops a Location with automatic storage > duration. Except it fails to pop when -icount parameter "rr" isn't > given. cur_loc then points to unused stack space, and will most > likely get clobbered in short order. > > Clobbered cur_loc can make loc_pop() and error_print_loc() crash or > report bogus locations. > > Broken in commit 890ad55. > > I didn't take the time to find a reproducer. > > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> Oops! Thanks for catching it. Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> -- Eduardo ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it 2016-04-27 14:29 [Qemu-devel] [PATCH for-2.6 0/3] Fix dangling pointers and error message regressions Markus Armbruster 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 1/3] QemuOpts: Fix qemu_opts_foreach() dangling location regression Markus Armbruster 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 2/3] replay: Fix dangling location bug in replay_configure() Markus Armbruster @ 2016-04-27 14:29 ` Markus Armbruster 2016-04-27 14:49 ` Daniel P. Berrange 2016-04-27 15:25 ` Eric Blake 2 siblings, 2 replies; 10+ messages in thread From: Markus Armbruster @ 2016-04-27 14:29 UTC (permalink / raw) To: qemu-devel qemu_opts_foreach() runs its callback with the error location set to the option's location. Any errors the callback reports use the option's location automatically. Commit 90998d5 moved the actual error reporting from "inside" qemu_opts_foreach() to after it. Here's a typical hunk: if (qemu_opts_foreach(qemu_find_opts("object"), - object_create, - object_create_initial, NULL)) { + user_creatable_add_opts_foreach, + object_create_initial, &err)) { + error_report_err(err); exit(1); } Before, object_create() reports from within qemu_opts_foreach(), using the option's location. Afterwards, we do it after qemu_opts_foreach(), using whatever location happens to be current. Commonly a "none" location. Reproducer: $ qemu-system-x86_64 -nodefaults -display none -object secret,id=foo,foo=bar qemu-system-x86_64: Property '.foo' not found Note no location. This commit restores it: qemu-system-x86_64: -object secret,id=foo,foo=bar: Property '.foo' not found Note that the qemu_opts_foreach() bug just fixed could mask the bug here: if the location it leaves dandling hasn't been clobbered, yet, it's the correct one. Reported-by: Eric Blake <eblake@redhat.com> Cc: Daniel P. Berrange <berrange@redhat.com> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qom/object_interfaces.h | 5 +++-- qemu-img.c | 39 +++++++++++---------------------------- qemu-io.c | 3 +-- qemu-nbd.c | 3 +-- qom/object_interfaces.c | 4 +++- vl.c | 6 ++---- 6 files changed, 21 insertions(+), 39 deletions(-) diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h index d579746..8b17f4d 100644 --- a/include/qom/object_interfaces.h +++ b/include/qom/object_interfaces.h @@ -140,7 +140,7 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type); * user_creatable_add_opts_foreach: * @opaque: a user_creatable_add_opts_predicate callback or NULL * @opts: options to create - * @errp: if an error occurs, a pointer to an area to store the error + * @errp: unused * * An iterator callback to be used in conjunction with * the qemu_opts_foreach() method for creating a list of @@ -148,8 +148,9 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type); * * The @opaque parameter can be passed a user_creatable_add_opts_predicate * callback to filter which types of object are created during iteration. + * When it fails, report the error. * - * Returns: 0 on success, -1 on error + * Returns: 0 on success, -1 when an error was reported. */ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp); diff --git a/qemu-img.c b/qemu-img.c index 1697762..46f2a6d 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -435,8 +435,7 @@ static int img_create(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { goto fail; } @@ -598,7 +597,6 @@ static int img_check(int argc, char **argv) bool writethrough; ImageCheck *check; bool quiet = false; - Error *local_err = NULL; bool image_opts = false; fmt = NULL; @@ -679,8 +677,7 @@ static int img_check(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { return 1; } @@ -871,8 +868,7 @@ static int img_commit(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { return 1; } @@ -1133,7 +1129,6 @@ static int img_compare(int argc, char **argv) int64_t nb_sectors; int c, pnum; uint64_t progress_base; - Error *local_err = NULL; bool image_opts = false; cache = BDRV_DEFAULT_CACHE; @@ -1201,8 +1196,7 @@ static int img_compare(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { ret = 2; goto out4; } @@ -1864,8 +1858,7 @@ static int img_convert(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { goto fail_getopt; } @@ -2299,7 +2292,6 @@ static int img_info(int argc, char **argv) bool chain = false; const char *filename, *fmt, *output; ImageInfoList *list; - Error *local_err = NULL; bool image_opts = false; fmt = NULL; @@ -2363,8 +2355,7 @@ static int img_info(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { return 1; } @@ -2513,7 +2504,6 @@ static int img_map(int argc, char **argv) int64_t length; MapEntry curr = { .length = 0 }, next; int ret = 0; - Error *local_err = NULL; bool image_opts = false; fmt = NULL; @@ -2573,8 +2563,7 @@ static int img_map(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { return 1; } @@ -2717,8 +2706,7 @@ static int img_snapshot(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &err)) { - error_report_err(err); + NULL, NULL)) { return 1; } @@ -2867,8 +2855,7 @@ static int img_rebase(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { return 1; } @@ -3133,7 +3120,6 @@ static int img_resize(int argc, char **argv) bool quiet = false; BlockBackend *blk = NULL; QemuOpts *param; - Error *local_err = NULL; static QemuOptsList resize_options = { .name = "resize_options", @@ -3204,8 +3190,7 @@ static int img_resize(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { return 1; } @@ -3297,7 +3282,6 @@ static int img_amend(int argc, char **argv) bool quiet = false, progress = false; BlockBackend *blk = NULL; BlockDriverState *bs = NULL; - Error *local_err = NULL; bool image_opts = false; cache = BDRV_DEFAULT_CACHE; @@ -3365,8 +3349,7 @@ static int img_amend(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { ret = -1; goto out_no_progress; } diff --git a/qemu-io.c b/qemu-io.c index 288bba8..0598251 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -534,8 +534,7 @@ int main(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_error)) { - error_report_err(local_error); + NULL, NULL)) { exit(1); } diff --git a/qemu-nbd.c b/qemu-nbd.c index 2c9754e..c55b40f 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -711,8 +711,7 @@ int main(int argc, char **argv) if (qemu_opts_foreach(&qemu_object_opts, user_creatable_add_opts_foreach, - NULL, &local_err)) { - error_report_err(local_err); + NULL, NULL)) { exit(EXIT_FAILURE); } diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index ab5da35..3931890 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -170,6 +170,7 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp) { bool (*type_predicate)(const char *) = opaque; Object *obj = NULL; + Error *err = NULL; const char *type; type = qemu_opt_get(opts, "qom-type"); @@ -178,8 +179,9 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp) return 0; } - obj = user_creatable_add_opts(opts, errp); + obj = user_creatable_add_opts(opts, &err); if (!obj) { + error_report_err(err); return -1; } object_unref(obj); diff --git a/vl.c b/vl.c index 9df534f..5fd22cb 100644 --- a/vl.c +++ b/vl.c @@ -4291,8 +4291,7 @@ int main(int argc, char **argv, char **envp) if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_add_opts_foreach, - object_create_initial, &err)) { - error_report_err(err); + object_create_initial, NULL)) { exit(1); } @@ -4410,8 +4409,7 @@ int main(int argc, char **argv, char **envp) if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_add_opts_foreach, - object_create_delayed, &err)) { - error_report_err(err); + object_create_delayed, NULL)) { exit(1); } -- 2.5.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it Markus Armbruster @ 2016-04-27 14:49 ` Daniel P. Berrange 2016-04-27 15:36 ` Markus Armbruster 2016-04-27 15:25 ` Eric Blake 1 sibling, 1 reply; 10+ messages in thread From: Daniel P. Berrange @ 2016-04-27 14:49 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, eblake On Wed, Apr 27, 2016 at 04:29:09PM +0200, Markus Armbruster wrote: > qemu_opts_foreach() runs its callback with the error location set to > the option's location. Any errors the callback reports use the > option's location automatically. > > Commit 90998d5 moved the actual error reporting from "inside" > qemu_opts_foreach() to after it. Here's a typical hunk: > > if (qemu_opts_foreach(qemu_find_opts("object"), > - object_create, > - object_create_initial, NULL)) { > + user_creatable_add_opts_foreach, > + object_create_initial, &err)) { > + error_report_err(err); > exit(1); > } > > Before, object_create() reports from within qemu_opts_foreach(), using > the option's location. Afterwards, we do it after > qemu_opts_foreach(), using whatever location happens to be current. > Commonly a "none" location. IMHO this shows a major design flaw with error_report_err() method and the location handling. The design pattern we have for "Error *" objects is that we can freely propagate them up the caller, because it is a self-contained record of the error information. As soon as you do that you loose the location information, because it was not in fact associated with the Error, but rather stored in a single global variable. For that matter, the Location info isn't even thread safe AFAICT since its a simple state var, so you better hope that there's no code which calls loc_push/pop from a non-main thread :-( > > Reproducer: > > $ qemu-system-x86_64 -nodefaults -display none -object secret,id=foo,foo=bar > qemu-system-x86_64: Property '.foo' not found > > Note no location. This commit restores it: > > qemu-system-x86_64: -object secret,id=foo,foo=bar: Property '.foo' not found > > Note that the qemu_opts_foreach() bug just fixed could mask the bug > here: if the location it leaves dandling hasn't been clobbered, yet, > it's the correct one. > > Reported-by: Eric Blake <eblake@redhat.com> > Cc: Daniel P. Berrange <berrange@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > include/qom/object_interfaces.h | 5 +++-- > qemu-img.c | 39 +++++++++++---------------------------- > qemu-io.c | 3 +-- > qemu-nbd.c | 3 +-- > qom/object_interfaces.c | 4 +++- > vl.c | 6 ++---- > 6 files changed, 21 insertions(+), 39 deletions(-) > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h > index d579746..8b17f4d 100644 > --- a/include/qom/object_interfaces.h > +++ b/include/qom/object_interfaces.h > @@ -140,7 +140,7 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type); > * user_creatable_add_opts_foreach: > * @opaque: a user_creatable_add_opts_predicate callback or NULL > * @opts: options to create > - * @errp: if an error occurs, a pointer to an area to store the error > + * @errp: unused > * > * An iterator callback to be used in conjunction with > * the qemu_opts_foreach() method for creating a list of > @@ -148,8 +148,9 @@ typedef bool (*user_creatable_add_opts_predicate)(const char *type); > * > * The @opaque parameter can be passed a user_creatable_add_opts_predicate > * callback to filter which types of object are created during iteration. > + * When it fails, report the error. > * > - * Returns: 0 on success, -1 on error > + * Returns: 0 on success, -1 when an error was reported. > */ > int user_creatable_add_opts_foreach(void *opaque, > QemuOpts *opts, Error **errp); > diff --git a/qemu-img.c b/qemu-img.c > index 1697762..46f2a6d 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -435,8 +435,7 @@ static int img_create(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > goto fail; > } > > @@ -598,7 +597,6 @@ static int img_check(int argc, char **argv) > bool writethrough; > ImageCheck *check; > bool quiet = false; > - Error *local_err = NULL; > bool image_opts = false; > > fmt = NULL; > @@ -679,8 +677,7 @@ static int img_check(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > return 1; > } > > @@ -871,8 +868,7 @@ static int img_commit(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > return 1; > } > > @@ -1133,7 +1129,6 @@ static int img_compare(int argc, char **argv) > int64_t nb_sectors; > int c, pnum; > uint64_t progress_base; > - Error *local_err = NULL; > bool image_opts = false; > > cache = BDRV_DEFAULT_CACHE; > @@ -1201,8 +1196,7 @@ static int img_compare(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > ret = 2; > goto out4; > } > @@ -1864,8 +1858,7 @@ static int img_convert(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > goto fail_getopt; > } > > @@ -2299,7 +2292,6 @@ static int img_info(int argc, char **argv) > bool chain = false; > const char *filename, *fmt, *output; > ImageInfoList *list; > - Error *local_err = NULL; > bool image_opts = false; > > fmt = NULL; > @@ -2363,8 +2355,7 @@ static int img_info(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > return 1; > } > > @@ -2513,7 +2504,6 @@ static int img_map(int argc, char **argv) > int64_t length; > MapEntry curr = { .length = 0 }, next; > int ret = 0; > - Error *local_err = NULL; > bool image_opts = false; > > fmt = NULL; > @@ -2573,8 +2563,7 @@ static int img_map(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > return 1; > } > > @@ -2717,8 +2706,7 @@ static int img_snapshot(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &err)) { > - error_report_err(err); > + NULL, NULL)) { > return 1; > } > > @@ -2867,8 +2855,7 @@ static int img_rebase(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > return 1; > } > > @@ -3133,7 +3120,6 @@ static int img_resize(int argc, char **argv) > bool quiet = false; > BlockBackend *blk = NULL; > QemuOpts *param; > - Error *local_err = NULL; > > static QemuOptsList resize_options = { > .name = "resize_options", > @@ -3204,8 +3190,7 @@ static int img_resize(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > return 1; > } > > @@ -3297,7 +3282,6 @@ static int img_amend(int argc, char **argv) > bool quiet = false, progress = false; > BlockBackend *blk = NULL; > BlockDriverState *bs = NULL; > - Error *local_err = NULL; > bool image_opts = false; > > cache = BDRV_DEFAULT_CACHE; > @@ -3365,8 +3349,7 @@ static int img_amend(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > ret = -1; > goto out_no_progress; > } > diff --git a/qemu-io.c b/qemu-io.c > index 288bba8..0598251 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -534,8 +534,7 @@ int main(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_error)) { > - error_report_err(local_error); > + NULL, NULL)) { > exit(1); > } > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 2c9754e..c55b40f 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -711,8 +711,7 @@ int main(int argc, char **argv) > > if (qemu_opts_foreach(&qemu_object_opts, > user_creatable_add_opts_foreach, > - NULL, &local_err)) { > - error_report_err(local_err); > + NULL, NULL)) { > exit(EXIT_FAILURE); > } > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c > index ab5da35..3931890 100644 > --- a/qom/object_interfaces.c > +++ b/qom/object_interfaces.c > @@ -170,6 +170,7 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp) > { > bool (*type_predicate)(const char *) = opaque; > Object *obj = NULL; > + Error *err = NULL; > const char *type; > > type = qemu_opt_get(opts, "qom-type"); > @@ -178,8 +179,9 @@ int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp) > return 0; > } > > - obj = user_creatable_add_opts(opts, errp); > + obj = user_creatable_add_opts(opts, &err); > if (!obj) { > + error_report_err(err); > return -1; > } > object_unref(obj); > diff --git a/vl.c b/vl.c > index 9df534f..5fd22cb 100644 > --- a/vl.c > +++ b/vl.c > @@ -4291,8 +4291,7 @@ int main(int argc, char **argv, char **envp) > > if (qemu_opts_foreach(qemu_find_opts("object"), > user_creatable_add_opts_foreach, > - object_create_initial, &err)) { > - error_report_err(err); > + object_create_initial, NULL)) { > exit(1); > } > > @@ -4410,8 +4409,7 @@ int main(int argc, char **argv, char **envp) > > if (qemu_opts_foreach(qemu_find_opts("object"), > user_creatable_add_opts_foreach, > - object_create_delayed, &err)) { > - error_report_err(err); > + object_create_delayed, NULL)) { > exit(1); > } Very reluctant Reviewed-by: Daniel P. Berrange <berrange@redhat.com> this really needs fixing properly in 2.7 so that the Error object is fully self contained so that later use of it does not rely on any global state. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it 2016-04-27 14:49 ` Daniel P. Berrange @ 2016-04-27 15:36 ` Markus Armbruster 0 siblings, 0 replies; 10+ messages in thread From: Markus Armbruster @ 2016-04-27 15:36 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: qemu-devel "Daniel P. Berrange" <berrange@redhat.com> writes: > On Wed, Apr 27, 2016 at 04:29:09PM +0200, Markus Armbruster wrote: >> qemu_opts_foreach() runs its callback with the error location set to >> the option's location. Any errors the callback reports use the >> option's location automatically. >> >> Commit 90998d5 moved the actual error reporting from "inside" >> qemu_opts_foreach() to after it. Here's a typical hunk: >> >> if (qemu_opts_foreach(qemu_find_opts("object"), >> - object_create, >> - object_create_initial, NULL)) { >> + user_creatable_add_opts_foreach, >> + object_create_initial, &err)) { >> + error_report_err(err); >> exit(1); >> } >> >> Before, object_create() reports from within qemu_opts_foreach(), using >> the option's location. Afterwards, we do it after >> qemu_opts_foreach(), using whatever location happens to be current. >> Commonly a "none" location. > > IMHO this shows a major design flaw with error_report_err() method > and the location handling. The design pattern we have for "Error *" > objects is that we can freely propagate them up the caller, because > it is a self-contained record of the error information. As soon as > you do that you loose the location information, because it was not > in fact associated with the Error, but rather stored in a single > global variable. For that matter, the Location info isn't even > thread safe AFAICT since its a simple state var, so you better hope > that there's no code which calls loc_push/pop from a non-main thread :-( I readily concede that the current state is decidedly sub-optimal. Error reporting in QEMU has a tortuous history, and it shows. Locations date back to simpler times. Threads? What's a "thread"? The current location stack was the simplest way to retrofit locations to most of the errors with the least churn. If it's a good idea (which is debatable), it should certainly be thread-local. Error was created with cavalier disregard for actual error messages. We've fixed the worst issues, but we haven't attacked location information. Instead, we fall back to what error_report() gives us for free: the current location at the point where we report the error. Blindly replacing this by the current location at the point where we detect the error may not always be an improvement. It depends. Here's an instructive example: -drive if=none,cache=none,file=blkdebug:blkdebug.conf:... with an erroneous blkdebug.conf. The current location at the point where we detect the error is the bad spot in blkdebug.conf. That's useful information. It currently gets lost. The current location at the point where we report the error should be the -drive (it currently isn't, but that's just a bug). Also useful information. >> Reproducer: >> >> $ qemu-system-x86_64 -nodefaults -display none -object secret,id=foo,foo=bar >> qemu-system-x86_64: Property '.foo' not found >> >> Note no location. This commit restores it: >> >> qemu-system-x86_64: -object secret,id=foo,foo=bar: Property '.foo' not found >> >> Note that the qemu_opts_foreach() bug just fixed could mask the bug >> here: if the location it leaves dandling hasn't been clobbered, yet, >> it's the correct one. >> >> Reported-by: Eric Blake <eblake@redhat.com> >> Cc: Daniel P. Berrange <berrange@redhat.com> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> [...] > > Very reluctant > > Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Thanks! > this really needs fixing properly in 2.7 so that the Error object is > fully self contained so that later use of it does not rely on any > global state. Worthwhile project. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it Markus Armbruster 2016-04-27 14:49 ` Daniel P. Berrange @ 2016-04-27 15:25 ` Eric Blake 1 sibling, 0 replies; 10+ messages in thread From: Eric Blake @ 2016-04-27 15:25 UTC (permalink / raw) To: Markus Armbruster, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1873 bytes --] On 04/27/2016 08:29 AM, Markus Armbruster wrote: > qemu_opts_foreach() runs its callback with the error location set to > the option's location. Any errors the callback reports use the > option's location automatically. > > Commit 90998d5 moved the actual error reporting from "inside" > qemu_opts_foreach() to after it. Here's a typical hunk: > > if (qemu_opts_foreach(qemu_find_opts("object"), > - object_create, > - object_create_initial, NULL)) { > + user_creatable_add_opts_foreach, > + object_create_initial, &err)) { > + error_report_err(err); > exit(1); > } > > Before, object_create() reports from within qemu_opts_foreach(), using > the option's location. Afterwards, we do it after > qemu_opts_foreach(), using whatever location happens to be current. > Commonly a "none" location. I agree with Dan that Error objects ought to track the Location in effect at the point the Error is first registered, rather than concatenating the two back together at the time the Error is eventually reported; but also that such a change is too big to even consider this late in 2.6. So as a band-aid, this particular patch improves the error message quality back to its useful state. Reviewed-by: Eric Blake <eblake@redhat.com> > Note that the qemu_opts_foreach() bug just fixed could mask the bug > here: if the location it leaves dandling hasn't been clobbered, yet, s/dandling/dangling/ > it's the correct one. > > Reported-by: Eric Blake <eblake@redhat.com> > Cc: Daniel P. Berrange <berrange@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-04-27 16:39 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-27 14:29 [Qemu-devel] [PATCH for-2.6 0/3] Fix dangling pointers and error message regressions Markus Armbruster 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 1/3] QemuOpts: Fix qemu_opts_foreach() dangling location regression Markus Armbruster 2016-04-27 14:42 ` Eric Blake 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 2/3] replay: Fix dangling location bug in replay_configure() Markus Armbruster 2016-04-27 14:57 ` Eric Blake 2016-04-27 16:39 ` Eduardo Habkost 2016-04-27 14:29 ` [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it Markus Armbruster 2016-04-27 14:49 ` Daniel P. Berrange 2016-04-27 15:36 ` Markus Armbruster 2016-04-27 15:25 ` Eric Blake
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.