| Message ID | 1268848614-6844-6-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > > > >
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. [...]
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.
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?
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.
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).
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.
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.
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); -}
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(-)