[Qemu-devel] [PATCH 0/6] error: Clean up after recent changes

[Qemu-devel] [PATCH 0/6] error: Clean up after recent changes

[Markus Armbruster]

Cleaner integration of location tracking with qemu-tool.c.  Move
qerror_report() where it belongs.

Markus Armbruster (6):
  error: Trim includes after "Move qemu_error & friends..."
  error: Trim includes in qerror.c
  error: Trim includes after "Infrastructure to track locations..."
  error: Make use of error_set_progname() optional
  error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o
  error: Move qerror_report() from qemu-error.[ch] to qerror.[ch]

 Makefile             |    6 +++---
 hw/qdev-properties.c |    1 +
 monitor.c            |    2 --
 monitor.h            |    1 -
 qemu-error.c         |   20 +-------------------
 qemu-error.h         |    6 ------
 qemu-tool.c          |   50 ++++++++++++++++----------------------------------
 qerror.c             |   22 ++++++++++++++++++++--
 qerror.h             |    5 +++++
 sysemu.h             |    2 --
 10 files changed, 46 insertions(+), 69 deletions(-)

[Qemu-devel] [PATCH 1/6] error: Trim includes after "Move qemu_error & friends..."

[Markus Armbruster]

Missed in commit 2f792016.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev-properties.c |    1 +
 monitor.c            |    2 --
 sysemu.h             |    2 --
 3 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 92d6793..157a111 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1,6 +1,7 @@
 #include "sysemu.h"
 #include "net.h"
 #include "qdev.h"
+#include "qerror.h"
 
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
 {
diff --git a/monitor.c b/monitor.c
index 0448a70..822dc27 100644
--- a/monitor.c
+++ b/monitor.c
@@ -49,10 +49,8 @@
 #include "qint.h"
 #include "qfloat.h"
 #include "qlist.h"
-#include "qdict.h"
 #include "qbool.h"
 #include "qstring.h"
-#include "qerror.h"
 #include "qjson.h"
 #include "json-streamer.h"
 #include "json-parser.h"
diff --git a/sysemu.h b/sysemu.h
index 8a9c630..9d3d51d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -6,8 +6,6 @@
 #include "qemu-option.h"
 #include "qemu-queue.h"
 #include "qemu-timer.h"
-#include "qdict.h"
-#include "qerror.h"
 
 #ifdef _WIN32
 #include <windows.h>
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/6] error: Trim includes in qerror.c

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/qerror.c b/qerror.c
index d0aba61..ff2fbd5 100644
--- a/qerror.c
+++ b/qerror.c
@@ -11,9 +11,7 @@
  */
 #include "qjson.h"
 #include "qerror.h"
-#include "qstring.h"
 #include "qemu-common.h"
-#include "qemu-error.h"
 
 static void qerror_destroy_obj(QObject *obj);
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/6] error: Trim includes after "Infrastructure to track locations..."

[Markus Armbruster]

Missed in commit 827b0813.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/monitor.h b/monitor.h
index bd4ae34..5bdeed1 100644
--- a/monitor.h
+++ b/monitor.h
@@ -3,7 +3,6 @@
 
 #include "qemu-common.h"
 #include "qemu-char.h"
-#include "qemu-error.h"
 #include "qerror.h"
 #include "qdict.h"
 #include "block.h"
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/6] error: Make use of error_set_progname() optional

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-error.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-error.c b/qemu-error.c
index 5d5fe37..9b9c0a1 100644
--- a/qemu-error.c
+++ b/qemu-error.c
@@ -167,7 +167,7 @@ void error_print_loc(void)
     int i;
     const char *const *argp;
 
-    if (!cur_mon) {
+    if (!cur_mon && progname) {
         fprintf(stderr, "%s:", progname);
         sep = " ";
     }
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Markus Armbruster]

The location tracking interface is used by code shared with qemi-img,
qemu-nbd and qemu-io, so it needs to be available there.  Commit
827b0813 provides it in a rather hamfisted way: it adds a dummy
implementation to qemu-tool.c.

It's cleaner to provide the real thing, and put a few more dummy
monitor functions into qemu-tool.c.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 Makefile    |    6 +++---
 qemu-tool.c |   50 ++++++++++++++++----------------------------------
 2 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/Makefile b/Makefile
index 2066c12..aad361e 100644
--- a/Makefile
+++ b/Makefile
@@ -129,11 +129,11 @@ bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
 qemu-img.o: qemu-img-cmds.h
 qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
 
-qemu-img$(EXESUF): qemu-img.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
+qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
 
-qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
+qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
 
-qemu-io$(EXESUF): qemu-io.o qemu-tool.o cmd.o $(block-obj-y) $(qobject-obj-y)
+qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
 
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 	$(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
diff --git a/qemu-tool.c b/qemu-tool.c
index 97ca949..8d6d0ef 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -15,7 +15,6 @@
 #include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-log.h"
-#include "qemu-error.h"
 
 #include <sys/time.h>
 
@@ -33,6 +32,22 @@ void qemu_service_io(void)
 {
 }
 
+Monitor *cur_mon;
+
+int monitor_cur_is_qmp(void)
+{
+    return 0;
+}
+
+void monitor_set_error(Monitor *mon, QError *qerror)
+{
+    assert(0);
+}
+
+void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
+{
+}
+
 void monitor_printf(Monitor *mon, const char *fmt, ...)
 {
 }
@@ -103,36 +118,3 @@ int64_t qemu_get_clock(QEMUClock *clock)
     qemu_gettimeofday(&tv);
     return (tv.tv_sec * 1000000000LL + (tv.tv_usec * 1000)) / 1000000;
 }
-
-Location *loc_push_restore(Location *loc)
-{
-    return loc;
-}
-
-Location *loc_push_none(Location *loc)
-{
-    return loc;
-}
-
-Location *loc_pop(Location *loc)
-{
-    return loc;
-}
-
-Location *loc_save(Location *loc)
-{
-    return loc;
-}
-
-void loc_restore(Location *loc)
-{
-}
-
-void error_report(const char *fmt, ...)
-{
-    va_list args;
-
-    va_start(args, fmt);
-    vfprintf(stderr, fmt, args);
-    va_end(args);
-}
-- 
1.6.6.1

[Qemu-devel] [PATCH 6/6] error: Move qerror_report() from qemu-error.[ch] to qerror.[ch]

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-error.c |   18 ------------------
 qemu-error.h |    6 ------
 qerror.c     |   20 ++++++++++++++++++++
 qerror.h     |    5 +++++
 4 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/qemu-error.c b/qemu-error.c
index 9b9c0a1..57d7555 100644
--- a/qemu-error.c
+++ b/qemu-error.c
@@ -207,21 +207,3 @@ void error_report(const char *fmt, ...)
     va_end(ap);
     error_printf("\n");
 }
-
-void qerror_report_internal(const char *file, int linenr, const char *func,
-                            const char *fmt, ...)
-{
-    va_list va;
-    QError *qerror;
-
-    va_start(va, fmt);
-    qerror = qerror_from_info(file, linenr, func, fmt, &va);
-    va_end(va);
-
-    if (monitor_cur_is_qmp()) {
-        monitor_set_error(cur_mon, qerror);
-    } else {
-        qerror_print(qerror);
-        QDECREF(qerror);
-    }
-}
diff --git a/qemu-error.h b/qemu-error.h
index e63c6ab..a45609f 100644
--- a/qemu-error.h
+++ b/qemu-error.h
@@ -37,11 +37,5 @@ void error_printf_unless_qmp(const char *fmt, ...)
 void error_print_loc(void);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
-void qerror_report_internal(const char *file, int linenr, const char *func,
-                            const char *fmt, ...)
-    __attribute__ ((format(printf, 4, 5)));
-
-#define qerror_report(fmt, ...) \
-    qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
 
 #endif
diff --git a/qerror.c b/qerror.c
index ff2fbd5..eaa1deb 100644
--- a/qerror.c
+++ b/qerror.c
@@ -9,6 +9,8 @@
  * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
  * See the COPYING.LIB file in the top-level directory.
  */
+
+#include "monitor.h"
 #include "qjson.h"
 #include "qerror.h"
 #include "qemu-common.h"
@@ -377,6 +379,24 @@ void qerror_print(QError *qerror)
     QDECREF(qstring);
 }
 
+void qerror_report_internal(const char *file, int linenr, const char *func,
+                            const char *fmt, ...)
+{
+    va_list va;
+    QError *qerror;
+
+    va_start(va, fmt);
+    qerror = qerror_from_info(file, linenr, func, fmt, &va);
+    va_end(va);
+
+    if (monitor_cur_is_qmp()) {
+        monitor_set_error(cur_mon, qerror);
+    } else {
+        qerror_print(qerror);
+        QDECREF(qerror);
+    }
+}
+
 /**
  * qobject_to_qerror(): Convert a QObject into a QError
  */
diff --git a/qerror.h b/qerror.h
index d96abe1..dd298d4 100644
--- a/qerror.h
+++ b/qerror.h
@@ -37,6 +37,11 @@ QError *qerror_from_info(const char *file, int linenr, const char *func,
                          const char *fmt, va_list *va);
 QString *qerror_human(const QError *qerror);
 void qerror_print(QError *qerror);
+void qerror_report_internal(const char *file, int linenr, const char *func,
+                            const char *fmt, ...)
+    __attribute__ ((format(printf, 4, 5)));
+#define qerror_report(fmt, ...) \
+    qerror_report_internal(__FILE__, __LINE__, __func__, fmt, ## __VA_ARGS__)
 QError *qobject_to_qerror(const QObject *obj);
 
 /*
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Blue Swirl]

On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
> The location tracking interface is used by code shared with qemi-img,
>  qemu-nbd and qemu-io, so it needs to be available there.  Commit
>  827b0813 provides it in a rather hamfisted way: it adds a dummy
>  implementation to qemu-tool.c.
>
>  It's cleaner to provide the real thing, and put a few more dummy
>  monitor functions into qemu-tool.c.
>
>  Signed-off-by: Markus Armbruster <armbru@redhat.com>
>  ---
>   Makefile    |    6 +++---
>   qemu-tool.c |   50 ++++++++++++++++----------------------------------
>   2 files changed, 19 insertions(+), 37 deletions(-)
>
>  diff --git a/Makefile b/Makefile
>  index 2066c12..aad361e 100644
>  --- a/Makefile
>  +++ b/Makefile
>  @@ -129,11 +129,11 @@ bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
>   qemu-img.o: qemu-img-cmds.h
>   qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
>
>  -qemu-img$(EXESUF): qemu-img.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>  +qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>
>  -qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>  +qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>
>  -qemu-io$(EXESUF): qemu-io.o qemu-tool.o cmd.o $(block-obj-y) $(qobject-obj-y)
>  +qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>
>   qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
>         $(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
>  diff --git a/qemu-tool.c b/qemu-tool.c
>  index 97ca949..8d6d0ef 100644
>  --- a/qemu-tool.c
>  +++ b/qemu-tool.c
>  @@ -15,7 +15,6 @@
>   #include "monitor.h"
>   #include "qemu-timer.h"
>   #include "qemu-log.h"
>  -#include "qemu-error.h"
>
>   #include <sys/time.h>
>
>  @@ -33,6 +32,22 @@ void qemu_service_io(void)
>   {
>   }
>
>  +Monitor *cur_mon;
>  +
>  +int monitor_cur_is_qmp(void)
>  +{
>  +    return 0;
>  +}
>  +
>  +void monitor_set_error(Monitor *mon, QError *qerror)
>  +{
>  +    assert(0);

Please use abort().

>  +}
>  +
>  +void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  +{
>  +}
>  +
>   void monitor_printf(Monitor *mon, const char *fmt, ...)
>   {
>   }
>  @@ -103,36 +118,3 @@ int64_t qemu_get_clock(QEMUClock *clock)
>      qemu_gettimeofday(&tv);
>      return (tv.tv_sec * 1000000000LL + (tv.tv_usec * 1000)) / 1000000;
>   }
>  -
>  -Location *loc_push_restore(Location *loc)
>  -{
>  -    return loc;
>  -}
>  -
>  -Location *loc_push_none(Location *loc)
>  -{
>  -    return loc;
>  -}
>  -
>  -Location *loc_pop(Location *loc)
>  -{
>  -    return loc;
>  -}
>  -
>  -Location *loc_save(Location *loc)
>  -{
>  -    return loc;
>  -}
>  -
>  -void loc_restore(Location *loc)
>  -{
>  -}
>  -
>  -void error_report(const char *fmt, ...)
>  -{
>  -    va_list args;
>  -
>  -    va_start(args, fmt);
>  -    vfprintf(stderr, fmt, args);
>  -    va_end(args);
>  -}
>
> --
>  1.6.6.1
>
>
>
>

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>> The location tracking interface is used by code shared with qemi-img,
>>  qemu-nbd and qemu-io, so it needs to be available there.  Commit
>>  827b0813 provides it in a rather hamfisted way: it adds a dummy
>>  implementation to qemu-tool.c.
>>
>>  It's cleaner to provide the real thing, and put a few more dummy
>>  monitor functions into qemu-tool.c.
>>
>>  Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>  ---
>>   Makefile    |    6 +++---
>>   qemu-tool.c |   50 ++++++++++++++++----------------------------------
>>   2 files changed, 19 insertions(+), 37 deletions(-)
>>
>>  diff --git a/Makefile b/Makefile
>>  index 2066c12..aad361e 100644
>>  --- a/Makefile
>>  +++ b/Makefile
>>  @@ -129,11 +129,11 @@ bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
>>   qemu-img.o: qemu-img-cmds.h
>>   qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
>>
>>  -qemu-img$(EXESUF): qemu-img.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>>  +qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>>
>>  -qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>>  +qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>>
>>  -qemu-io$(EXESUF): qemu-io.o qemu-tool.o cmd.o $(block-obj-y) $(qobject-obj-y)
>>  +qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>>
>>   qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
>>         $(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
>>  diff --git a/qemu-tool.c b/qemu-tool.c
>>  index 97ca949..8d6d0ef 100644
>>  --- a/qemu-tool.c
>>  +++ b/qemu-tool.c
>>  @@ -15,7 +15,6 @@
>>   #include "monitor.h"
>>   #include "qemu-timer.h"
>>   #include "qemu-log.h"
>>  -#include "qemu-error.h"
>>
>>   #include <sys/time.h>
>>
>>  @@ -33,6 +32,22 @@ void qemu_service_io(void)
>>   {
>>   }
>>
>>  +Monitor *cur_mon;
>>  +
>>  +int monitor_cur_is_qmp(void)
>>  +{
>>  +    return 0;
>>  +}
>>  +
>>  +void monitor_set_error(Monitor *mon, QError *qerror)
>>  +{
>>  +    assert(0);
>
> Please use abort().

Why?

I'd like to understand the rules to avoid unnecessary respins in the
future.

[...]

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Blue Swirl]

On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>  >> The location tracking interface is used by code shared with qemi-img,
>  >>  qemu-nbd and qemu-io, so it needs to be available there.  Commit
>  >>  827b0813 provides it in a rather hamfisted way: it adds a dummy
>  >>  implementation to qemu-tool.c.
>  >>
>  >>  It's cleaner to provide the real thing, and put a few more dummy
>  >>  monitor functions into qemu-tool.c.
>  >>
>  >>  Signed-off-by: Markus Armbruster <armbru@redhat.com>
>  >>  ---
>  >>   Makefile    |    6 +++---
>  >>   qemu-tool.c |   50 ++++++++++++++++----------------------------------
>  >>   2 files changed, 19 insertions(+), 37 deletions(-)
>  >>
>  >>  diff --git a/Makefile b/Makefile
>  >>  index 2066c12..aad361e 100644
>  >>  --- a/Makefile
>  >>  +++ b/Makefile
>  >>  @@ -129,11 +129,11 @@ bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
>  >>   qemu-img.o: qemu-img-cmds.h
>  >>   qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
>  >>
>  >>  -qemu-img$(EXESUF): qemu-img.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>  >>  +qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>  >>
>  >>  -qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o $(block-obj-y) $(qobject-obj-y)
>  >>  +qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>  >>
>  >>  -qemu-io$(EXESUF): qemu-io.o qemu-tool.o cmd.o $(block-obj-y) $(qobject-obj-y)
>  >>  +qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobject-obj-y)
>  >>
>  >>   qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
>  >>         $(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
>  >>  diff --git a/qemu-tool.c b/qemu-tool.c
>  >>  index 97ca949..8d6d0ef 100644
>  >>  --- a/qemu-tool.c
>  >>  +++ b/qemu-tool.c
>  >>  @@ -15,7 +15,6 @@
>  >>   #include "monitor.h"
>  >>   #include "qemu-timer.h"
>  >>   #include "qemu-log.h"
>  >>  -#include "qemu-error.h"
>  >>
>  >>   #include <sys/time.h>
>  >>
>  >>  @@ -33,6 +32,22 @@ void qemu_service_io(void)
>  >>   {
>  >>   }
>  >>
>  >>  +Monitor *cur_mon;
>  >>  +
>  >>  +int monitor_cur_is_qmp(void)
>  >>  +{
>  >>  +    return 0;
>  >>  +}
>  >>  +
>  >>  +void monitor_set_error(Monitor *mon, QError *qerror)
>  >>  +{
>  >>  +    assert(0);
>  >
>  > Please use abort().
>
>
> Why?

Because assert(0) does not abort when compiled with -DNDEBUG.

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
[...]
>>  >>  +void monitor_set_error(Monitor *mon, QError *qerror)
>>  >>  +{
>>  >>  +    assert(0);
>>  >
>>  > Please use abort().
>>
>>
>> Why?
>
> Because assert(0) does not abort when compiled with -DNDEBUG.

Why is that a problem?  And why isn't it a problem for the 300+ other
assertions in the code?

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Blue Swirl]

On 3/18/10, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>  >> Blue Swirl <blauwirbel@gmail.com> writes:
>  >>
>  >>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>
> [...]
>
> >>  >>  +void monitor_set_error(Monitor *mon, QError *qerror)
>  >>  >>  +{
>  >>  >>  +    assert(0);
>  >>  >
>  >>  > Please use abort().
>  >>
>  >>
>  >> Why?
>  >
>  > Because assert(0) does not abort when compiled with -DNDEBUG.
>
>
> Why is that a problem?  And why isn't it a problem for the 300+ other
>  assertions in the code?

We didn't support -DNDEBUG build until recently. Therefore it's safe
to assume that also on production builds assert(0) was equal to
abort(). If the default for production builds changes to -DNDEBUG, we
want to retain the same abort() behaviour.

The assertions which perform debugging checks aren't a problem. But
assert(0) clearly isn't a debugging check, if you ever reach this line
of code, it's abort() time.

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On 3/18/10, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>>  >> Blue Swirl <blauwirbel@gmail.com> writes:
>>  >>
>>  >>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> [...]
>>
>> >>  >>  +void monitor_set_error(Monitor *mon, QError *qerror)
>>  >>  >>  +{
>>  >>  >>  +    assert(0);
>>  >>  >
>>  >>  > Please use abort().
>>  >>
>>  >>
>>  >> Why?
>>  >
>>  > Because assert(0) does not abort when compiled with -DNDEBUG.
>>
>>
>> Why is that a problem?  And why isn't it a problem for the 300+ other
>>  assertions in the code?
>
> We didn't support -DNDEBUG build until recently. Therefore it's safe
> to assume that also on production builds assert(0) was equal to
> abort(). If the default for production builds changes to -DNDEBUG, we
> want to retain the same abort() behaviour.
>
> The assertions which perform debugging checks aren't a problem. But
> assert(0) clearly isn't a debugging check, if you ever reach this line
> of code, it's abort() time.

I *know* that this line cannot be reached.  That's why I asserted it
cannot be reached.

If you want "this can't ever happen" assertions to be checked, then you
shouldn't define NDEBUG.

Do you still want me to use abort() here?

By the way, a quick grep shows >50 uses of assert(0).

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Blue Swirl]

On 3/18/10, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>  > On 3/18/10, Markus Armbruster <armbru@redhat.com> wrote:
>  >> Blue Swirl <blauwirbel@gmail.com> writes:
>  >>
>  >>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>  >>  >> Blue Swirl <blauwirbel@gmail.com> writes:
>  >>  >>
>  >>  >>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>  >>
>  >> [...]
>  >>
>  >> >>  >>  +void monitor_set_error(Monitor *mon, QError *qerror)
>  >>  >>  >>  +{
>  >>  >>  >>  +    assert(0);
>  >>  >>  >
>  >>  >>  > Please use abort().
>  >>  >>
>  >>  >>
>  >>  >> Why?
>  >>  >
>  >>  > Because assert(0) does not abort when compiled with -DNDEBUG.
>  >>
>  >>
>  >> Why is that a problem?  And why isn't it a problem for the 300+ other
>  >>  assertions in the code?
>  >
>  > We didn't support -DNDEBUG build until recently. Therefore it's safe
>  > to assume that also on production builds assert(0) was equal to
>  > abort(). If the default for production builds changes to -DNDEBUG, we
>  > want to retain the same abort() behaviour.
>  >
>  > The assertions which perform debugging checks aren't a problem. But
>  > assert(0) clearly isn't a debugging check, if you ever reach this line
>  > of code, it's abort() time.
>
>
> I *know* that this line cannot be reached.  That's why I asserted it
>  cannot be reached.
>
>  If you want "this can't ever happen" assertions to be checked, then you
>  shouldn't define NDEBUG.

If you know for sure that this line can't be reached, why bother with
any code at all?

>  Do you still want me to use abort() here?

abort() or nothing at all, as you wish.

>  By the way, a quick grep shows >50 uses of assert(0).

Not anymore since 43dc2a645e00e6761a741e3d16c27c5b5a373b66.

[Qemu-devel] Re: [PATCH 1/6] error: Trim includes after "Move qemu_error & friends..."

[Luiz Capitulino]

On Wed, 17 Mar 2010 18:56:49 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Missed in commit 2f792016.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/qdev-properties.c |    1 +
>  monitor.c            |    2 --
>  sysemu.h             |    2 --
>  3 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 92d6793..157a111 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -1,6 +1,7 @@
>  #include "sysemu.h"
>  #include "net.h"
>  #include "qdev.h"
> +#include "qerror.h"
>  
>  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
>  {
> diff --git a/monitor.c b/monitor.c
> index 0448a70..822dc27 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -49,10 +49,8 @@
>  #include "qint.h"
>  #include "qfloat.h"
>  #include "qlist.h"
> -#include "qdict.h"
>  #include "qbool.h"
>  #include "qstring.h"
> -#include "qerror.h"
>  #include "qjson.h"
>  #include "json-streamer.h"
>  #include "json-parser.h"
> diff --git a/sysemu.h b/sysemu.h
> index 8a9c630..9d3d51d 100644

 Hmm, why this second hunk? Also note that we have qemu-objects.h.

> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -6,8 +6,6 @@
>  #include "qemu-option.h"
>  #include "qemu-queue.h"
>  #include "qemu-timer.h"
> -#include "qdict.h"
> -#include "qerror.h"
>  
>  #ifdef _WIN32
>  #include <windows.h>

[Qemu-devel] Re: [PATCH 2/6] error: Trim includes in qerror.c

[Luiz Capitulino]

On Wed, 17 Mar 2010 18:56:50 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qerror.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/qerror.c b/qerror.c
> index d0aba61..ff2fbd5 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -11,9 +11,7 @@
>   */
>  #include "qjson.h"
>  #include "qerror.h"
> -#include "qstring.h"
>  #include "qemu-common.h"
> -#include "qemu-error.h"

 Care to explain the reason? Are you relaying on the includes in qjson.h?

>  
>  static void qerror_destroy_obj(QObject *obj);
>  

[Qemu-devel] Re: [PATCH 1/6] error: Trim includes after "Move qemu_error & friends..."

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 17 Mar 2010 18:56:49 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Missed in commit 2f792016.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/qdev-properties.c |    1 +
>>  monitor.c            |    2 --
>>  sysemu.h             |    2 --
>>  3 files changed, 1 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
>> index 92d6793..157a111 100644
>> --- a/hw/qdev-properties.c
>> +++ b/hw/qdev-properties.c
>> @@ -1,6 +1,7 @@
>>  #include "sysemu.h"
>>  #include "net.h"
>>  #include "qdev.h"
>> +#include "qerror.h"
>>  
>>  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
>>  {
>> diff --git a/monitor.c b/monitor.c
>> index 0448a70..822dc27 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -49,10 +49,8 @@
>>  #include "qint.h"
>>  #include "qfloat.h"
>>  #include "qlist.h"
>> -#include "qdict.h"
>>  #include "qbool.h"
>>  #include "qstring.h"
>> -#include "qerror.h"
>>  #include "qjson.h"
>>  #include "json-streamer.h"
>>  #include "json-parser.h"
>> diff --git a/sysemu.h b/sysemu.h
>> index 8a9c630..9d3d51d 100644
>
>  Hmm, why this second hunk? Also note that we have qemu-objects.h.

The one below or the one above?

What do you mean by "we have qemu-objects.h"?

>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -6,8 +6,6 @@
>>  #include "qemu-option.h"
>>  #include "qemu-queue.h"
>>  #include "qemu-timer.h"
>> -#include "qdict.h"
>> -#include "qerror.h"
>>  
>>  #ifdef _WIN32
>>  #include <windows.h>

[Qemu-devel] Re: [PATCH 2/6] error: Trim includes in qerror.c

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 17 Mar 2010 18:56:50 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qerror.c |    2 --
>>  1 files changed, 0 insertions(+), 2 deletions(-)
>> 
>> diff --git a/qerror.c b/qerror.c
>> index d0aba61..ff2fbd5 100644
>> --- a/qerror.c
>> +++ b/qerror.c
>> @@ -11,9 +11,7 @@
>>   */
>>  #include "qjson.h"
>>  #include "qerror.h"
>> -#include "qstring.h"
>>  #include "qemu-common.h"
>> -#include "qemu-error.h"
>
>  Care to explain the reason? Are you relaying on the includes in qjson.h?

qerror.h already includes them.

[...]

[Qemu-devel] Re: [PATCH 2/6] error: Trim includes in qerror.c

[Luiz Capitulino]

On Fri, 19 Mar 2010 22:33:29 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 17 Mar 2010 18:56:50 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  qerror.c |    2 --
> >>  1 files changed, 0 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/qerror.c b/qerror.c
> >> index d0aba61..ff2fbd5 100644
> >> --- a/qerror.c
> >> +++ b/qerror.c
> >> @@ -11,9 +11,7 @@
> >>   */
> >>  #include "qjson.h"
> >>  #include "qerror.h"
> >> -#include "qstring.h"
> >>  #include "qemu-common.h"
> >> -#include "qemu-error.h"
> >
> >  Care to explain the reason? Are you relaying on the includes in qjson.h?
> 
> qerror.h already includes them.

 Oh, okay then.

[Qemu-devel] Re: [PATCH 1/6] error: Trim includes after "Move qemu_error & friends..."

[Luiz Capitulino]

On Fri, 19 Mar 2010 22:31:05 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 17 Mar 2010 18:56:49 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Missed in commit 2f792016.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  hw/qdev-properties.c |    1 +
> >>  monitor.c            |    2 --
> >>  sysemu.h             |    2 --
> >>  3 files changed, 1 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> >> index 92d6793..157a111 100644
> >> --- a/hw/qdev-properties.c
> >> +++ b/hw/qdev-properties.c
> >> @@ -1,6 +1,7 @@
> >>  #include "sysemu.h"
> >>  #include "net.h"
> >>  #include "qdev.h"
> >> +#include "qerror.h"
> >>  
> >>  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
> >>  {
> >> diff --git a/monitor.c b/monitor.c
> >> index 0448a70..822dc27 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -49,10 +49,8 @@
> >>  #include "qint.h"
> >>  #include "qfloat.h"
> >>  #include "qlist.h"
> >> -#include "qdict.h"
> >>  #include "qbool.h"
> >>  #include "qstring.h"
> >> -#include "qerror.h"
> >>  #include "qjson.h"
> >>  #include "json-streamer.h"
> >>  #include "json-parser.h"
> >> diff --git a/sysemu.h b/sysemu.h
> >> index 8a9c630..9d3d51d 100644
> >
> >  Hmm, why this second hunk? Also note that we have qemu-objects.h.
> 
> The one below or the one above?

 The one above, but I believe it's because monitor.h includes them,
right? This is fine.

> What do you mean by "we have qemu-objects.h"?

 That header includes all QObject header files, so if you want do
some cleanup you could include it and drop all q-ones.

> 
> >> --- a/sysemu.h
> >> +++ b/sysemu.h
> >> @@ -6,8 +6,6 @@
> >>  #include "qemu-option.h"
> >>  #include "qemu-queue.h"
> >>  #include "qemu-timer.h"
> >> -#include "qdict.h"
> >> -#include "qerror.h"
> >>  
> >>  #ifdef _WIN32
> >>  #include <windows.h>

[Qemu-devel] Re: [PATCH 1/6] error: Trim includes after "Move qemu_error & friends..."

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 19 Mar 2010 22:31:05 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Wed, 17 Mar 2010 18:56:49 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Missed in commit 2f792016.
>> >> 
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  hw/qdev-properties.c |    1 +
>> >>  monitor.c            |    2 --
>> >>  sysemu.h             |    2 --
>> >>  3 files changed, 1 insertions(+), 4 deletions(-)
>> >> 
>> >> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
>> >> index 92d6793..157a111 100644
>> >> --- a/hw/qdev-properties.c
>> >> +++ b/hw/qdev-properties.c
>> >> @@ -1,6 +1,7 @@
>> >>  #include "sysemu.h"
>> >>  #include "net.h"
>> >>  #include "qdev.h"
>> >> +#include "qerror.h"
>> >>  
>> >>  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
>> >>  {
>> >> diff --git a/monitor.c b/monitor.c
>> >> index 0448a70..822dc27 100644
>> >> --- a/monitor.c
>> >> +++ b/monitor.c
>> >> @@ -49,10 +49,8 @@
>> >>  #include "qint.h"
>> >>  #include "qfloat.h"
>> >>  #include "qlist.h"
>> >> -#include "qdict.h"
>> >>  #include "qbool.h"
>> >>  #include "qstring.h"
>> >> -#include "qerror.h"
>> >>  #include "qjson.h"
>> >>  #include "json-streamer.h"
>> >>  #include "json-parser.h"
>> >> diff --git a/sysemu.h b/sysemu.h
>> >> index 8a9c630..9d3d51d 100644
>> >
>> >  Hmm, why this second hunk? Also note that we have qemu-objects.h.
>> 
>> The one below or the one above?
>
>  The one above, but I believe it's because monitor.h includes them,
> right? This is fine.

Right.

[...]

Re: [Qemu-devel] [PATCH 5/6] error: Link qemu-img, qemu-nbd, qemu-io with qemu-error.o

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On 3/18/10, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>  > On 3/18/10, Markus Armbruster <armbru@redhat.com> wrote:
>>  >> Blue Swirl <blauwirbel@gmail.com> writes:
>>  >>
>>  >>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>>  >>  >> Blue Swirl <blauwirbel@gmail.com> writes:
>>  >>  >>
>>  >>  >>  > On 3/17/10, Markus Armbruster <armbru@redhat.com> wrote:
>>  >>
>>  >> [...]
>>  >>
>>  >> >>  >>  +void monitor_set_error(Monitor *mon, QError *qerror)
>>  >>  >>  >>  +{
>>  >>  >>  >>  +    assert(0);
>>  >>  >>  >
>>  >>  >>  > Please use abort().
>>  >>  >>
>>  >>  >>
>>  >>  >> Why?
>>  >>  >
>>  >>  > Because assert(0) does not abort when compiled with -DNDEBUG.
>>  >>
>>  >>
>>  >> Why is that a problem?  And why isn't it a problem for the 300+ other
>>  >>  assertions in the code?
>>  >
>>  > We didn't support -DNDEBUG build until recently. Therefore it's safe
>>  > to assume that also on production builds assert(0) was equal to
>>  > abort(). If the default for production builds changes to -DNDEBUG, we
>>  > want to retain the same abort() behaviour.
>>  >
>>  > The assertions which perform debugging checks aren't a problem. But
>>  > assert(0) clearly isn't a debugging check, if you ever reach this line
>>  > of code, it's abort() time.
>>
>>
>> I *know* that this line cannot be reached.  That's why I asserted it
>>  cannot be reached.
>>
>>  If you want "this can't ever happen" assertions to be checked, then you
>>  shouldn't define NDEBUG.
>
> If you know for sure that this line can't be reached, why bother with
> any code at all?
>
>>  Do you still want me to use abort() here?
>
> abort() or nothing at all, as you wish.

Okay, nothing it is.

>>  By the way, a quick grep shows >50 uses of assert(0).
>
> Not anymore since 43dc2a645e00e6761a741e3d16c27c5b5a373b66.

I doubt the wisdom of such a wholesale conversion, but I it's hardly
worth arguing now.