| Message ID | 1470901038-9409-2-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
Peter Xu <peterx@redhat.com> writes: > There are many places in current QEMU codes that needs to print some > error and then quit QEMU. Provide a macro for it. > > Also, one coccinelle script is added to convert existing cases to > leverage this new macro. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/qemu/error-report.h | 8 ++++++++ > scripts/coccinelle/error_report_exit.cocci | 13 +++++++++++++ > 2 files changed, 21 insertions(+) > create mode 100644 scripts/coccinelle/error_report_exit.cocci > > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h > index 499ec8b..c2d5059 100644 > --- a/include/qemu/error-report.h > +++ b/include/qemu/error-report.h > @@ -13,6 +13,8 @@ > #ifndef QEMU_ERROR_REPORT_H > #define QEMU_ERROR_REPORT_H > > +#include <unistd.h> > + > typedef struct Location { > /* all members are private to qemu-error.c */ > enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind; > @@ -36,6 +38,12 @@ void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > void error_set_progname(const char *argv0); > void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > + > +#define error_report_exit(...) do { \ > + error_report(__VA_ARGS__); \ > + exit(1); \ > + } while (0) > + Let's call it error_report_fatal(), for consistency with error_fatal. Also, please make it a function, not a macro: void error_report_fatal(const char *fmt, ...) { va_list ap; va_start(ap, fmt); error_vreport(fmt, ap); va_end(ap); exit(1); } > const char *error_get_progname(void); > extern bool enable_timestamp_msg; > > diff --git a/scripts/coccinelle/error_report_exit.cocci b/scripts/coccinelle/error_report_exit.cocci > new file mode 100644 > index 0000000..49f6c17 > --- /dev/null > +++ b/scripts/coccinelle/error_report_exit.cocci > @@ -0,0 +1,13 @@ > +@@ > +expression list X; > +@@ > + > +-error_report(X); > ++error_report_exit(X); > +-exit( > +( > +-1 > +| > +-EXIT_FAILURE > +) > +-);
On Tue, 08/16 13:05, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > There are many places in current QEMU codes that needs to print some > > error and then quit QEMU. Provide a macro for it. > > > > Also, one coccinelle script is added to convert existing cases to > > leverage this new macro. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > include/qemu/error-report.h | 8 ++++++++ > > scripts/coccinelle/error_report_exit.cocci | 13 +++++++++++++ > > 2 files changed, 21 insertions(+) > > create mode 100644 scripts/coccinelle/error_report_exit.cocci > > > > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h > > index 499ec8b..c2d5059 100644 > > --- a/include/qemu/error-report.h > > +++ b/include/qemu/error-report.h > > @@ -13,6 +13,8 @@ > > #ifndef QEMU_ERROR_REPORT_H > > #define QEMU_ERROR_REPORT_H > > > > +#include <unistd.h> > > + > > typedef struct Location { > > /* all members are private to qemu-error.c */ > > enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind; > > @@ -36,6 +38,12 @@ void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > void error_set_progname(const char *argv0); > > void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > > void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > + > > +#define error_report_exit(...) do { \ > > + error_report(__VA_ARGS__); \ > > + exit(1); \ > > + } while (0) > > + > > Let's call it error_report_fatal(), for consistency with error_fatal. Do we really need error_report_exit when we already have error_fatal? Fam > > Also, please make it a function, not a macro: > > void error_report_fatal(const char *fmt, ...) > { > va_list ap; > > va_start(ap, fmt); > error_vreport(fmt, ap); > va_end(ap); > exit(1); > } > > > const char *error_get_progname(void); > > extern bool enable_timestamp_msg; > > > > diff --git a/scripts/coccinelle/error_report_exit.cocci b/scripts/coccinelle/error_report_exit.cocci > > new file mode 100644 > > index 0000000..49f6c17 > > --- /dev/null > > +++ b/scripts/coccinelle/error_report_exit.cocci > > @@ -0,0 +1,13 @@ > > +@@ > > +expression list X; > > +@@ > > + > > +-error_report(X); > > ++error_report_exit(X); > > +-exit( > > +( > > +-1 > > +| > > +-EXIT_FAILURE > > +) > > +-); >
On Tue, Aug 16, 2016 at 07:53:51PM +0800, Fam Zheng wrote: > On Tue, 08/16 13:05, Markus Armbruster wrote: > > Peter Xu <peterx@redhat.com> writes: > > > > > There are many places in current QEMU codes that needs to print some > > > error and then quit QEMU. Provide a macro for it. > > > > > > Also, one coccinelle script is added to convert existing cases to > > > leverage this new macro. > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > > include/qemu/error-report.h | 8 ++++++++ > > > scripts/coccinelle/error_report_exit.cocci | 13 +++++++++++++ > > > 2 files changed, 21 insertions(+) > > > create mode 100644 scripts/coccinelle/error_report_exit.cocci > > > > > > diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h > > > index 499ec8b..c2d5059 100644 > > > --- a/include/qemu/error-report.h > > > +++ b/include/qemu/error-report.h > > > @@ -13,6 +13,8 @@ > > > #ifndef QEMU_ERROR_REPORT_H > > > #define QEMU_ERROR_REPORT_H > > > > > > +#include <unistd.h> > > > + > > > typedef struct Location { > > > /* all members are private to qemu-error.c */ > > > enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind; > > > @@ -36,6 +38,12 @@ void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > > void error_set_progname(const char *argv0); > > > void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); > > > void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); > > > + > > > +#define error_report_exit(...) do { \ > > > + error_report(__VA_ARGS__); \ > > > + exit(1); \ > > > + } while (0) > > > + > > > > Let's call it error_report_fatal(), for consistency with error_fatal. Sure. Actually I don't know whether we will like this serie. Anyway, let me prepare a v2. > > Do we really need error_report_exit when we already have error_fatal? error_fatal is the name of a global var, not the function. > > > > Also, please make it a function, not a macro: > > > > void error_report_fatal(const char *fmt, ...) > > { > > va_list ap; > > > > va_start(ap, fmt); > > error_vreport(fmt, ap); > > va_end(ap); > > exit(1); > > } Ok. Thanks! -- peterx
On Tue, 08/16 20:17, Peter Xu wrote: > > Do we really need error_report_exit when we already have error_fatal? > > error_fatal is the name of a global var, not the function. I mean most error_report_exit(...) calls can be converted to error_setg(&error_fatal, ...). Fam
On Tue, Aug 16, 2016 at 08:19:08PM +0800, Fam Zheng wrote: > On Tue, 08/16 20:17, Peter Xu wrote: > > > Do we really need error_report_exit when we already have error_fatal? > > > > error_fatal is the name of a global var, not the function. > > I mean most error_report_exit(...) calls can be converted to > error_setg(&error_fatal, ...). Right. But it's just another way to implement error_report_fatal(). We may still need error_report_fatal() since it's cleaner and shorter than error_setg(&error_fatal, ...). -- peterx
On Tue, 08/16 22:00, Peter Xu wrote: > On Tue, Aug 16, 2016 at 08:19:08PM +0800, Fam Zheng wrote: > > On Tue, 08/16 20:17, Peter Xu wrote: > > > > Do we really need error_report_exit when we already have error_fatal? > > > > > > error_fatal is the name of a global var, not the function. > > > > I mean most error_report_exit(...) calls can be converted to > > error_setg(&error_fatal, ...). > > Right. But it's just another way to implement error_report_fatal(). We > may still need error_report_fatal() since it's cleaner and shorter > than error_setg(&error_fatal, ...). Then we should add both error_report_fatal and error_report_abort, or neither of them. Fam
Peter Xu <peterx@redhat.com> writes: > On Tue, Aug 16, 2016 at 08:19:08PM +0800, Fam Zheng wrote: >> On Tue, 08/16 20:17, Peter Xu wrote: >> > > Do we really need error_report_exit when we already have error_fatal? >> > >> > error_fatal is the name of a global var, not the function. >> >> I mean most error_report_exit(...) calls can be converted to >> error_setg(&error_fatal, ...). > > Right. But it's just another way to implement error_report_fatal(). We > may still need error_report_fatal() since it's cleaner and shorter > than error_setg(&error_fatal, ...). Fam's point that error_report_fatal(...); is just another way to write error_setg(&error_fatal, ...) is valid. Your point that the former is shorter and simpler is also valid. In fact, error.h advises: * Please don't error_setg(&error_fatal, ...), use error_report() and * exit(), because that's more obvious. * Likewise, don't error_setg(&error_abort, ...), use assert(). Could you convert the existing error_setg(&error_fatal, ...) to error_report_fatal(...)? Regarding error_report_abort(): in my opinion, printing pretty messages right before abort() is largely a waste of time. But if people insist on doing it, then the error subsystem may have to support it. Would you be willing to track down such usage, so we can make an informed decision?
On Tue, Aug 16, 2016 at 04:45:13PM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Tue, Aug 16, 2016 at 08:19:08PM +0800, Fam Zheng wrote: > >> On Tue, 08/16 20:17, Peter Xu wrote: > >> > > Do we really need error_report_exit when we already have error_fatal? > >> > > >> > error_fatal is the name of a global var, not the function. > >> > >> I mean most error_report_exit(...) calls can be converted to > >> error_setg(&error_fatal, ...). > > > > Right. But it's just another way to implement error_report_fatal(). We > > may still need error_report_fatal() since it's cleaner and shorter > > than error_setg(&error_fatal, ...). > > Fam's point that > > error_report_fatal(...); > > is just another way to write > > error_setg(&error_fatal, ...) > > is valid. Your point that the former is shorter and simpler is also > valid. In fact, error.h advises: > > * Please don't error_setg(&error_fatal, ...), use error_report() and > * exit(), because that's more obvious. > * Likewise, don't error_setg(&error_abort, ...), use assert(). > > Could you convert the existing error_setg(&error_fatal, ...) to > error_report_fatal(...)? > > Regarding error_report_abort(): in my opinion, printing pretty messages > right before abort() is largely a waste of time. But if people insist > on doing it, then the error subsystem may have to support it. Would you > be willing to track down such usage, so we can make an informed > decision? Sure. :) Yes I see usages for error_setg(&error_abort, ...), it makes sense to provide error_report_abort() along with error_report_fatal(). Will take your (and Fam's) advice. Thanks for your comments! -- peterx
Peter Xu <peterx@redhat.com> writes: > On Tue, Aug 16, 2016 at 04:45:13PM +0200, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > On Tue, Aug 16, 2016 at 08:19:08PM +0800, Fam Zheng wrote: >> >> On Tue, 08/16 20:17, Peter Xu wrote: >> >> > > Do we really need error_report_exit when we already have error_fatal? >> >> > >> >> > error_fatal is the name of a global var, not the function. >> >> >> >> I mean most error_report_exit(...) calls can be converted to >> >> error_setg(&error_fatal, ...). >> > >> > Right. But it's just another way to implement error_report_fatal(). We >> > may still need error_report_fatal() since it's cleaner and shorter >> > than error_setg(&error_fatal, ...). >> >> Fam's point that >> >> error_report_fatal(...); >> >> is just another way to write >> >> error_setg(&error_fatal, ...) >> >> is valid. Your point that the former is shorter and simpler is also >> valid. In fact, error.h advises: >> >> * Please don't error_setg(&error_fatal, ...), use error_report() and >> * exit(), because that's more obvious. >> * Likewise, don't error_setg(&error_abort, ...), use assert(). >> >> Could you convert the existing error_setg(&error_fatal, ...) to >> error_report_fatal(...)? >> >> Regarding error_report_abort(): in my opinion, printing pretty messages >> right before abort() is largely a waste of time. But if people insist >> on doing it, then the error subsystem may have to support it. Would you >> be willing to track down such usage, so we can make an informed >> decision? > > Sure. :) > > Yes I see usages for error_setg(&error_abort, ...), it makes sense to > provide error_report_abort() along with error_report_fatal(). Will > take your (and Fam's) advice. Recommend to make it a separate patch. > Thanks for your comments! You're welcome!
On Wed, Aug 17, 2016 at 09:33:37AM +0200, Markus Armbruster wrote: > > Yes I see usages for error_setg(&error_abort, ...), it makes sense to > > provide error_report_abort() along with error_report_fatal(). Will > > take your (and Fam's) advice. > > Recommend to make it a separate patch. Will do. -- peterx
On Tue, Aug 16, 2016 at 08:17:08PM +0800, Peter Xu wrote: > > > > > > Also, please make it a function, not a macro: > > > > > > void error_report_fatal(const char *fmt, ...) > > > { > > > va_list ap; > > > > > > va_start(ap, fmt); > > > error_vreport(fmt, ap); > > > va_end(ap); > > > exit(1); > > > } Marcel (and reviewers), Now if we are having both error_report_fatal() and error_report_abort(), we'll write error_report() three times if we all take them as functions. How about we still use macro this time but leverage error_setg() macro as mentioned by Fam, like: +#define error_report_fatal(...) error_setg(&error_fatal, __VA_ARGS__) +#define error_report_abort(...) error_setg(&error_abort, __VA_ARGS__) In this case, we avoided calling exit() directly in the macro, and is much cleaner than writting error_report() content for three times. -- peterx
Peter Xu <peterx@redhat.com> writes: > On Tue, Aug 16, 2016 at 08:17:08PM +0800, Peter Xu wrote: >> > > >> > > Also, please make it a function, not a macro: >> > > >> > > void error_report_fatal(const char *fmt, ...) >> > > { >> > > va_list ap; >> > > >> > > va_start(ap, fmt); >> > > error_vreport(fmt, ap); >> > > va_end(ap); >> > > exit(1); >> > > } > > Marcel (and reviewers), > > Now if we are having both error_report_fatal() and > error_report_abort(), we'll write error_report() three times if we all > take them as functions. Yes, but all we duplicate is the usual var-arg boilerplate. > How about we still use macro this time but leverage error_setg() > macro as mentioned by Fam, like: > > +#define error_report_fatal(...) error_setg(&error_fatal, __VA_ARGS__) > +#define error_report_abort(...) error_setg(&error_abort, __VA_ARGS__) > > In this case, we avoided calling exit() directly in the macro, and is > much cleaner than writting error_report() content for three times. I'm afraid that destroys the layering. Currently, Error objects (util/error.c) use error reporting (util/qemu-error.c), but not vice versa. Their headers (include/qapi/error.h and include/qemu/error-report.h) are idependent. I like it that way.
On Wed, Aug 17, 2016 at 11:26:51AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Tue, Aug 16, 2016 at 08:17:08PM +0800, Peter Xu wrote: > >> > > > >> > > Also, please make it a function, not a macro: > >> > > > >> > > void error_report_fatal(const char *fmt, ...) > >> > > { > >> > > va_list ap; > >> > > > >> > > va_start(ap, fmt); > >> > > error_vreport(fmt, ap); > >> > > va_end(ap); > >> > > exit(1); > >> > > } > > > > Marcel (and reviewers), > > > > Now if we are having both error_report_fatal() and > > error_report_abort(), we'll write error_report() three times if we all > > take them as functions. > > Yes, but all we duplicate is the usual var-arg boilerplate. > > > How about we still use macro this time but leverage error_setg() > > macro as mentioned by Fam, like: > > > > +#define error_report_fatal(...) error_setg(&error_fatal, __VA_ARGS__) > > +#define error_report_abort(...) error_setg(&error_abort, __VA_ARGS__) > > > > In this case, we avoided calling exit() directly in the macro, and is > > much cleaner than writting error_report() content for three times. > > I'm afraid that destroys the layering. > > Currently, Error objects (util/error.c) use error reporting > (util/qemu-error.c), but not vice versa. Their headers > (include/qapi/error.h and include/qemu/error-report.h) are idependent. > I like it that way. I see. Then let me keep them functions. Thanks, -- peterx
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h index 499ec8b..c2d5059 100644 --- a/include/qemu/error-report.h +++ b/include/qemu/error-report.h @@ -13,6 +13,8 @@ #ifndef QEMU_ERROR_REPORT_H #define QEMU_ERROR_REPORT_H +#include <unistd.h> + typedef struct Location { /* all members are private to qemu-error.c */ enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind; @@ -36,6 +38,12 @@ void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2); void error_set_progname(const char *argv0); void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); + +#define error_report_exit(...) do { \ + error_report(__VA_ARGS__); \ + exit(1); \ + } while (0) + const char *error_get_progname(void); extern bool enable_timestamp_msg; diff --git a/scripts/coccinelle/error_report_exit.cocci b/scripts/coccinelle/error_report_exit.cocci new file mode 100644 index 0000000..49f6c17 --- /dev/null +++ b/scripts/coccinelle/error_report_exit.cocci @@ -0,0 +1,13 @@ +@@ +expression list X; +@@ + +-error_report(X); ++error_report_exit(X); +-exit( +( +-1 +| +-EXIT_FAILURE +) +-);
There are many places in current QEMU codes that needs to print some error and then quit QEMU. Provide a macro for it. Also, one coccinelle script is added to convert existing cases to leverage this new macro. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/qemu/error-report.h | 8 ++++++++ scripts/coccinelle/error_report_exit.cocci | 13 +++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 scripts/coccinelle/error_report_exit.cocci