Re: [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it

[Markus Armbruster]

"Daniel P. Berrange"  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, )) {
>> +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 
>> Cc: Daniel P. Berrange 
>> Signed-off-by: Markus Armbruster 
[...]
>
> Very reluctant
>
>  Reviewed-by: Daniel P. Berrange 

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.

Re: [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it

[Eric Blake]

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, )) {
> +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 


> 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 
> Cc: Daniel P. Berrange 
> Signed-off-by: Markus Armbruster 
> ---


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it

[Daniel P. Berrange]

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, )) {
> +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 
> Cc: Daniel P. Berrange 
> Signed-off-by: Markus Armbruster 
> ---
>  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(_object_opts,
>user_creatable_add_opts_foreach,
> -  NULL, _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(_object_opts,
>user_creatable_add_opts_foreach,
> -  NULL, _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(_object_opts,
>user_creatable_add_opts_foreach,
> -  NULL, _err)) {
> -

[Qemu-devel] [PATCH for-2.6 3/3] qom: -object error messages lost location, restore it

[Markus Armbruster]

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, )) {
+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 
Cc: Daniel P. Berrange 
Signed-off-by: Markus Armbruster 
---
 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(_object_opts,
   user_creatable_add_opts_foreach,
-  NULL, _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(_object_opts,
   user_creatable_add_opts_foreach,
-  NULL, _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(_object_opts,
   user_creatable_add_opts_foreach,
-  NULL, _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(_object_opts,
   user_creatable_add_opts_foreach,
-  NULL, _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(_object_opts,
   user_creatable_add_opts_foreach,
-  NULL, _err)) {
-