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

login
register
mail settings
Submitter Markus Armbruster
Date March 17, 2010, 5:56 p.m.
Message ID <1268848614-6844-6-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/47966/
State New
Headers show

Comments

Markus Armbruster - March 17, 2010, 5:56 p.m.
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(-)
Blue Swirl - March 17, 2010, 7:53 p.m.
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
>
>
>
>
Markus Armbruster - March 17, 2010, 9:17 p.m.
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.

[...]
Blue Swirl - March 18, 2010, 5:58 p.m.
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.
Markus Armbruster - March 18, 2010, 6:09 p.m.
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?
Blue Swirl - March 18, 2010, 6:20 p.m.
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.
Markus Armbruster - March 18, 2010, 6:55 p.m.
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).
Blue Swirl - March 18, 2010, 7:32 p.m.
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.
Markus Armbruster - March 22, 2010, 9:37 a.m.
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.

Patch

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);
-}