[Qemu-devel] [PATCH v1 1/2] tests: Add libqemustub to qom-interface-check

[Qemu-devel] [PATCH v1 1/2] tests: Add libqemustub to qom-interface-check

[Peter Crosthwaite]

The recent addition of util/error.c's dependency on error_report()
causes this test to fail to link due to a number of missing monitor
related symbols. All these symbols are however defined by libqemustub.
Add this libary to the link.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 tests/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/Makefile b/tests/Makefile
index 0aaf657..fd36eee 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -151,7 +151,7 @@ tests/check-qdict$(EXESUF): tests/check-qdict.o libqemuutil.a
 tests/check-qlist$(EXESUF): tests/check-qlist.o libqemuutil.a
 tests/check-qfloat$(EXESUF): tests/check-qfloat.o libqemuutil.a
 tests/check-qjson$(EXESUF): tests/check-qjson.o libqemuutil.a libqemustub.a
-tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a
+tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a libqemustub.a
 tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-throttle$(EXESUF): tests/test-throttle.o $(block-obj-y) libqemuutil.a libqemustub.a
-- 
1.8.5.3

[Qemu-devel] [PATCH v1 2/2] Revert "error: Don't use error_report() for assertion msgs."

[Peter Crosthwaite]

This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d.

The original implementation before this patch makes abortive error
messages much more friendly. The underlying bug that required this
change is now fixed. Revert.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 util/error.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/error.c b/util/error.c
index e5de34f..f11f1d5 100644
--- a/util/error.c
+++ b/util/error.c
@@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_report("%s", error_get_pretty(err));
         abort();
     }
 
@@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_report("%s", error_get_pretty(err));
         abort();
     }
 
@@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
     err->err_class = err_class;
 
     if (errp == &error_abort) {
-        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_report("%s", error_get_pretty(err));
         abort();
     }
 
@@ -171,7 +171,7 @@ void error_free(Error *err)
 void error_propagate(Error **dst_err, Error *local_err)
 {
     if (local_err && dst_err == &error_abort) {
-        fprintf(stderr, "%s\n", error_get_pretty(local_err));
+        error_report("%s", error_get_pretty(local_err));
         abort();
     } else if (dst_err && !*dst_err) {
         *dst_err = local_err;
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v1 2/2] Revert "error: Don't use error_report() for assertion msgs."

[Peter Crosthwaite]

Ping!

Do reverts require the usual reviewed-by? Markus are you happy with
this mini series to address your error_report() concerns?

Regards,
Peter

On Wed, Jan 15, 2014 at 9:06 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d.
>
> The original implementation before this patch makes abortive error
> messages much more friendly. The underlying bug that required this
> change is now fixed. Revert.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  util/error.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/util/error.c b/util/error.c
> index e5de34f..f11f1d5 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      err->err_class = err_class;
>
>      if (errp == &error_abort) {
> -        fprintf(stderr, "%s\n", error_get_pretty(err));
> +        error_report("%s", error_get_pretty(err));
>          abort();
>      }
>
> @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>      err->err_class = err_class;
>
>      if (errp == &error_abort) {
> -        fprintf(stderr, "%s\n", error_get_pretty(err));
> +        error_report("%s", error_get_pretty(err));
>          abort();
>      }
>
> @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
>      err->err_class = err_class;
>
>      if (errp == &error_abort) {
> -        fprintf(stderr, "%s\n", error_get_pretty(err));
> +        error_report("%s", error_get_pretty(err));
>          abort();
>      }
>
> @@ -171,7 +171,7 @@ void error_free(Error *err)
>  void error_propagate(Error **dst_err, Error *local_err)
>  {
>      if (local_err && dst_err == &error_abort) {
> -        fprintf(stderr, "%s\n", error_get_pretty(local_err));
> +        error_report("%s", error_get_pretty(local_err));
>          abort();
>      } else if (dst_err && !*dst_err) {
>          *dst_err = local_err;
> --
> 1.8.5.3
>

Re: [Qemu-devel] [PATCH v1 2/2] Revert "error: Don't use error_report() for assertion msgs."

[Edgar E. Iglesias]

Hi,

I committed it last week, forgot to email you.

Cheers,
Edgar

On Tue, Jan 28, 2014 at 12:35:08PM +1000, Peter Crosthwaite wrote:
> Ping!
> 
> Do reverts require the usual reviewed-by? Markus are you happy with
> this mini series to address your error_report() concerns?
> 
> Regards,
> Peter
> 
> On Wed, Jan 15, 2014 at 9:06 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
> > This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d.
> >
> > The original implementation before this patch makes abortive error
> > messages much more friendly. The underlying bug that required this
> > change is now fixed. Revert.
> >
> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > ---
> >
> >  util/error.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/util/error.c b/util/error.c
> > index e5de34f..f11f1d5 100644
> > --- a/util/error.c
> > +++ b/util/error.c
> > @@ -44,7 +44,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
> >      err->err_class = err_class;
> >
> >      if (errp == &error_abort) {
> > -        fprintf(stderr, "%s\n", error_get_pretty(err));
> > +        error_report("%s", error_get_pretty(err));
> >          abort();
> >      }
> >
> > @@ -80,7 +80,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> >      err->err_class = err_class;
> >
> >      if (errp == &error_abort) {
> > -        fprintf(stderr, "%s\n", error_get_pretty(err));
> > +        error_report("%s", error_get_pretty(err));
> >          abort();
> >      }
> >
> > @@ -125,7 +125,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
> >      err->err_class = err_class;
> >
> >      if (errp == &error_abort) {
> > -        fprintf(stderr, "%s\n", error_get_pretty(err));
> > +        error_report("%s", error_get_pretty(err));
> >          abort();
> >      }
> >
> > @@ -171,7 +171,7 @@ void error_free(Error *err)
> >  void error_propagate(Error **dst_err, Error *local_err)
> >  {
> >      if (local_err && dst_err == &error_abort) {
> > -        fprintf(stderr, "%s\n", error_get_pretty(local_err));
> > +        error_report("%s", error_get_pretty(local_err));
> >          abort();
> >      } else if (dst_err && !*dst_err) {
> >          *dst_err = local_err;
> > --
> > 1.8.5.3
> >
> 

Re: [Qemu-devel] [PATCH v1 2/2] Revert "error: Don't use error_report() for assertion msgs."

[Markus Armbruster]

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> Ping!
>
> Do reverts require the usual reviewed-by? Markus are you happy with
> this mini series to address your error_report() concerns?

Yes, I'm happy with it.  Thank you!

Re: [Qemu-devel] [PATCH v1 2/2] Revert "error: Don't use error_report() for assertion msgs."

[Peter Maydell]

On 15 January 2014 11:06, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d.
>
> The original implementation before this patch makes abortive error
> messages much more friendly. The underlying bug that required this
> change is now fixed. Revert.

Unfortunately 'make check' still doesn't work on MacOS:

cc -m64 -DOS_OBJECT_USE_OBJC=0 -arch x86_64 -D_GNU_SOURCE
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes
-fno-strict-aliasing  -Wno-initializer-overrides -Wendif-labels
-Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition
-Wtype-limits -fstack-protector-all -I/sw/include
-I/sw/include/libpng15   -I/sw/include   -I/opt/X11/include/pixman-1
-I/Users/pm215/src/qemu/dtc/libfdt -I/sw/include/glib-2.0
-I/sw/lib/glib-2.0/include -I/Users/pm215/src/qemu/tests -O2
-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g  -m64 -framework
CoreFoundation -framework IOKit -arch x86_64 -g  -o tests/check-qjson
tests/check-qjson.o libqemuutil.a libqemustub.a  -L/sw/lib
-lgthread-2.0 -lglib-2.0 -lintl  -lz -L/sw/lib -lssh2 -L/sw/lib -lcurl
Undefined symbols for architecture x86_64:
  "_cur_mon", referenced from:
      _error_vprintf in libqemuutil.a(qemu-error.o)
      _error_printf in libqemuutil.a(qemu-error.o)
      _error_printf_unless_qmp in libqemuutil.a(qemu-error.o)
      _error_print_loc in libqemuutil.a(qemu-error.o)
      _error_report in libqemuutil.a(qemu-error.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [tests/check-qjson] Error 1


Curiously, if you edit the cc command line so it says not just
"libqemustub.a" but "libqemustub.a stubs/mon-set-error.o" then
it links successfully. nm says that that .o file is in the .a:

libqemustub.a(mon-set-error.o):
0000000000000a68 s EH_frame0
                 U ___stack_chk_fail
                 U ___stack_chk_guard
0000000000000008 C _cur_mon
0000000000000000 T _monitor_set_error
0000000000000a80 S _monitor_set_error.eh

This appears to be a deficiency in the MacOSX linker and/or
executable format:

http://stackoverflow.com/questions/19398742/os-x-linker-unable-to-find-symbols-from-a-c-file-which-only-contains-variables

To fix this we need to:
 * make the cur_mon in the stub-file not common (eg by initializing it)
 * make the cur_mon in monitor.c not common (because otherwise
   the linker will prefer to pull in the version from the stub file, which
   then causes a clash between the stub monitor_set_error() function
   and the real one)

Which is kind of ugly.

The other approach would be to ensure that the monitor-related
stubs are combined into fewer stub .c files, such that any binary
that needs cur_mon will pull in the stub .o file because it needed
some function from it anyway.

Related questions:

why does util/qemu-error.c guard its use of cur_mon only by
"if (cur_mon)" whereas qobject/qerror.c uses "if (monitor_cur_is_qmp())"?

why, given that qerror.c has made a specific decision not to
send its output to the monitor, is it ok for error_report and
error_vprintf to override it and send the output to the monitor
anyway? if error_vprintf is correct should we remove the
higher level check?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 2/2] Revert "error: Don't use error_report() for assertion msgs."

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 15 January 2014 11:06, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> This reverts commit d32934c84c72f57e78d430c22974677b7bcabe5d.
>>
>> The original implementation before this patch makes abortive error
>> messages much more friendly. The underlying bug that required this
>> change is now fixed. Revert.
>
> Unfortunately 'make check' still doesn't work on MacOS:
>
> cc -m64 -DOS_OBJECT_USE_OBJC=0 -arch x86_64 -D_GNU_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
> -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes
> -fno-strict-aliasing  -Wno-initializer-overrides -Wendif-labels
> -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security
> -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition
> -Wtype-limits -fstack-protector-all -I/sw/include
> -I/sw/include/libpng15   -I/sw/include   -I/opt/X11/include/pixman-1
> -I/Users/pm215/src/qemu/dtc/libfdt -I/sw/include/glib-2.0
> -I/sw/lib/glib-2.0/include -I/Users/pm215/src/qemu/tests -O2
> -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g  -m64 -framework
> CoreFoundation -framework IOKit -arch x86_64 -g  -o tests/check-qjson
> tests/check-qjson.o libqemuutil.a libqemustub.a  -L/sw/lib
> -lgthread-2.0 -lglib-2.0 -lintl  -lz -L/sw/lib -lssh2 -L/sw/lib -lcurl
> Undefined symbols for architecture x86_64:
>   "_cur_mon", referenced from:
>       _error_vprintf in libqemuutil.a(qemu-error.o)
>       _error_printf in libqemuutil.a(qemu-error.o)
>       _error_printf_unless_qmp in libqemuutil.a(qemu-error.o)
>       _error_print_loc in libqemuutil.a(qemu-error.o)
>       _error_report in libqemuutil.a(qemu-error.o)
> ld: symbol(s) not found for architecture x86_64
> clang: error: linker command failed with exit code 1 (use -v to see invocation)
> make: *** [tests/check-qjson] Error 1
>
>
> Curiously, if you edit the cc command line so it says not just
> "libqemustub.a" but "libqemustub.a stubs/mon-set-error.o" then
> it links successfully. nm says that that .o file is in the .a:
>
> libqemustub.a(mon-set-error.o):
> 0000000000000a68 s EH_frame0
>                  U ___stack_chk_fail
>                  U ___stack_chk_guard
> 0000000000000008 C _cur_mon
> 0000000000000000 T _monitor_set_error
> 0000000000000a80 S _monitor_set_error.eh
>
> This appears to be a deficiency in the MacOSX linker and/or
> executable format:
>
> http://stackoverflow.com/questions/19398742/os-x-linker-unable-to-find-symbols-from-a-c-file-which-only-contains-variables
>
> To fix this we need to:
>  * make the cur_mon in the stub-file not common (eg by initializing it)
>  * make the cur_mon in monitor.c not common (because otherwise
>    the linker will prefer to pull in the version from the stub file, which
>    then causes a clash between the stub monitor_set_error() function
>    and the real one)
>
> Which is kind of ugly.

Not ugly, but a sensible move (in my opinion) regardless of this
specific issue: compile with -fno-common.  Then both become not common.

`-fno-common'
     In C code, controls the placement of uninitialized global
     variables.  Unix C compilers have traditionally permitted multiple
     definitions of such variables in different compilation units by
     placing the variables in a common block.  This is the behavior
     specified by `-fcommon', and is the default for GCC on most
     targets.  On the other hand, this behavior is not required by ISO
     C, and on some targets may carry a speed or code size penalty on
     variable references.  The `-fno-common' option specifies that the
     compiler should place uninitialized global variables in the data
     section of the object file, rather than generating them as common
     blocks.  This has the effect that if the same variable is declared
     (without `extern') in two different compilations, you will get a
     multiple-definition error when you link them.  In this case, you
     must compile with `-fcommon' instead.  Compiling with
     `-fno-common' is useful on targets for which it provides better
     performance, or if you wish to verify that the program will work
     on other systems that always treat uninitialized variable
     declarations this way.

> The other approach would be to ensure that the monitor-related
> stubs are combined into fewer stub .c files, such that any binary
> that needs cur_mon will pull in the stub .o file because it needed
> some function from it anyway.

That may make sense regardless.

> Related questions:
>
> why does util/qemu-error.c guard its use of cur_mon only by
> "if (cur_mon)" whereas qobject/qerror.c uses "if (monitor_cur_is_qmp())"?
>
> why, given that qerror.c has made a specific decision not to
> send its output to the monitor, is it ok for error_report and
> error_vprintf to override it and send the output to the monitor
> anyway? if error_vprintf is correct should we remove the
> higher level check?

Our prolongued thrashings in the error infrastructure quicksand have
resulted in a bit of a mess, I'm afraid.

We want to print human-readable messages to stderr in non-monitor
context, to the current monitor in HMP context, and not at all in QMP
context.

Why the latter?  Isn't an attempt to print to QMP simply a bug?  Yes, it
is.  However, when you call existing code from a QMP command handler,
finding every print that might reach is hard.  Hard means there will be
oversights.  Unless we guard against them, they'll kill the QMP
connection, so we better do.

The guard has evolved over time.  See, for instance, commit 10e4f60 and
commit 74ee59a.  Today, it works like this: at the deepest possible
point in the print-to-monitor call chain, namely monitor_vprintf(), we
silently do nothing unless we're in HMP contect.

Print-to-monitor functions further up the chain can remain oblivious of
the need for HMP.  Since they can, they should.

Print-to-the-right-place functions still need to decide whether to print
to the monitor or to stderr.  Again, this happens at the deepest
possible point in the call chain: error_vprintf().

qerror_report() is a different beast entirely.  It's the obsolete error
reporting API designed for minimally invasive conversion from existing
"just print the error" practice, saving us from having to rework call
chains to pass up error objects, with lots of fine opportunities to leak
them along the way.  Unfortunately, we later had to accept that there is
no way around that, and so we got error_set().

qerror_report() is still around, though.  It works like this: print the
error to the right place right away, *except* in QMP context.  There,
store an error object in the monitor, for later sending to the client.

Clear as mud?

Re: [Qemu-devel] [PATCH v1 2/2] Revert "error: Don't use error_report() for assertion msgs."

[Peter Maydell]

On 30 January 2014 08:00, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> [MacOSX doesn't pull in common symbols from .o files in
>> .a archives]

> Not ugly, but a sensible move (in my opinion) regardless of this
> specific issue: compile with -fno-common.  Then both become not common.
>
> `-fno-common'
>      In C code, controls the placement of uninitialized global
>      variables.  Unix C compilers have traditionally permitted multiple
>      definitions of such variables in different compilation units by
>      placing the variables in a common block.  This is the behavior
>      specified by `-fcommon', and is the default for GCC on most
>      targets.  On the other hand, this behavior is not required by ISO
>      C, and on some targets may carry a speed or code size penalty on
>      variable references.  The `-fno-common' option specifies that the
>      compiler should place uninitialized global variables in the data
>      section of the object file, rather than generating them as common
>      blocks.  This has the effect that if the same variable is declared
>      (without `extern') in two different compilations, you will get a
>      multiple-definition error when you link them.  In this case, you
>      must compile with `-fcommon' instead.  Compiling with
>      `-fno-common' is useful on targets for which it provides better
>      performance, or if you wish to verify that the program will work
>      on other systems that always treat uninitialized variable
>      declarations this way.

This seems to work. Judging by that description, we could build
with -fno-common everywhere. That would mean that accidentally
declaring the same variable in two compilation units was a compile
failure everywhere rather than just on MacOSX, which I think is
preferable.

Is there any reason we shouldn't just build with -fno-common
for all platforms and compilers?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 1/2] tests: Add libqemustub to qom-interface-check

[Igor Mammedov]

On Wed, 15 Jan 2014 03:05:36 -0800
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> The recent addition of util/error.c's dependency on error_report()
> causes this test to fail to link due to a number of missing monitor
> related symbols. All these symbols are however defined by libqemustub.
> Add this libary to the link.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  tests/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 0aaf657..fd36eee 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -151,7 +151,7 @@ tests/check-qdict$(EXESUF): tests/check-qdict.o libqemuutil.a
>  tests/check-qlist$(EXESUF): tests/check-qlist.o libqemuutil.a
>  tests/check-qfloat$(EXESUF): tests/check-qfloat.o libqemuutil.a
>  tests/check-qjson$(EXESUF): tests/check-qjson.o libqemuutil.a libqemustub.a
> -tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a
> +tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a libqemustub.a
>  tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil.a libqemustub.a
>  tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
>  tests/test-throttle$(EXESUF): tests/test-throttle.o $(block-obj-y) libqemuutil.a libqemustub.a
> -- 
> 1.8.5.3
> 

Reviewed-By: Igor Mammedov <imammedo@redhat.com>

-- 
Regards,
  Igor