| Message ID | 4e1eded5cda7b182a8a4cb133b40b2915817b7d1.1498596157.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 27.06.2017 22:45, Alistair Francis wrote: > Add a functino which can be used similarly to error_report() execpt to > inform the users about warnings instead of errors. s/functino/function/ s/execpt/except/ And could we maybe call the function "warn_report" or something else instead? I often already have trouble with error_report() to fit the string into one line of the scarce 80 columns space ... so the shorter the name of the function, the more characters can be squeezed into the string in one line later ;-) Thomas
On Tue, Jun 27, 2017 at 3:21 PM, Thomas Huth <thuth@redhat.com> wrote: > On 27.06.2017 22:45, Alistair Francis wrote: >> Add a functino which can be used similarly to error_report() execpt to >> inform the users about warnings instead of errors. > > s/functino/function/ > s/execpt/except/ Thanks, I'll fix these two in the next version. > > And could we maybe call the function "warn_report" or something else > instead? I often already have trouble with error_report() to fit the > string into one line of the scarce 80 columns space ... so the shorter > the name of the function, the more characters can be squeezed into the > string in one line later ;-) Good idea, I'll change it to warn_report(). Thanks, Alistair > > Thomas
On Tue, Jun 27, 2017 at 01:45:45PM -0700, Alistair Francis wrote: > Add a functino which can be used similarly to error_report() execpt to > inform the users about warnings instead of errors. > > The warning print does not include the timestamp and instead will > preface the messages with a 'warning: '. Not including the timestamp is a bug IMHO. If I've turned on timestamps, I expect all messages to have the timestamp. I'm not particularly convinced by adding the 'warning: ' prefix either, particularly given the scenario you are using this in, is not actually a warning - its just a informative message. Regards, Daniel
On Wed, Jun 28, 2017 at 2:04 AM, Daniel P. Berrange <berrange@redhat.com> wrote: > On Tue, Jun 27, 2017 at 01:45:45PM -0700, Alistair Francis wrote: >> Add a functino which can be used similarly to error_report() execpt to >> inform the users about warnings instead of errors. >> >> The warning print does not include the timestamp and instead will >> preface the messages with a 'warning: '. > > Not including the timestamp is a bug IMHO. If I've turned on timestamps, > I expect all messages to have the timestamp. That's fine, I'm happy to add it back in. I just wasn't sure. > > I'm not particularly convinced by adding the 'warning: ' prefix either, > particularly given the scenario you are using this in, is not actually > a warning - its just a informative message. Maybe it makes more sense to add an extra argument to error_report() that can be used to specify error, warning or information. The same way qemu_log_mask() works. That was Edgar's idea in reply to one of the other patches. Does that sound more useful? Thanks, Alistair > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jun 28, 2017 at 09:16:45AM -0700, Alistair Francis wrote: > On Wed, Jun 28, 2017 at 2:04 AM, Daniel P. Berrange <berrange@redhat.com> wrote: > > On Tue, Jun 27, 2017 at 01:45:45PM -0700, Alistair Francis wrote: > >> Add a functino which can be used similarly to error_report() execpt to > >> inform the users about warnings instead of errors. > >> > >> The warning print does not include the timestamp and instead will > >> preface the messages with a 'warning: '. > > > > Not including the timestamp is a bug IMHO. If I've turned on timestamps, > > I expect all messages to have the timestamp. > > That's fine, I'm happy to add it back in. I just wasn't sure. > > > > > I'm not particularly convinced by adding the 'warning: ' prefix either, > > particularly given the scenario you are using this in, is not actually > > a warning - its just a informative message. > > Maybe it makes more sense to add an extra argument to error_report() > that can be used to specify error, warning or information. The same > way qemu_log_mask() works. That was Edgar's idea in reply to one of > the other patches. > > Does that sound more useful? I'd suggest renaming the current 'error_report' to 'message_report' and making it take an extra arg that accepts a enum flag INFO | WARNING | ERROR. Then add macros for error_report, warning_report, info_report that call message_report with the right enum. That way you don't have to update any of the existing code that calls error_report. Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Wed, Jun 28, 2017 at 09:16:45AM -0700, Alistair Francis wrote: >> On Wed, Jun 28, 2017 at 2:04 AM, Daniel P. Berrange <berrange@redhat.com> wrote: >> > On Tue, Jun 27, 2017 at 01:45:45PM -0700, Alistair Francis wrote: >> >> Add a functino which can be used similarly to error_report() execpt to >> >> inform the users about warnings instead of errors. >> >> >> >> The warning print does not include the timestamp and instead will >> >> preface the messages with a 'warning: '. >> > >> > Not including the timestamp is a bug IMHO. If I've turned on timestamps, >> > I expect all messages to have the timestamp. >> >> That's fine, I'm happy to add it back in. I just wasn't sure. >> >> > >> > I'm not particularly convinced by adding the 'warning: ' prefix either, >> > particularly given the scenario you are using this in, is not actually >> > a warning - its just a informative message. >> >> Maybe it makes more sense to add an extra argument to error_report() >> that can be used to specify error, warning or information. The same >> way qemu_log_mask() works. That was Edgar's idea in reply to one of >> the other patches. >> >> Does that sound more useful? > > I'd suggest renaming the current 'error_report' to 'message_report' and > making it take an extra arg that accepts a enum flag INFO | WARNING | ERROR. > Then add macros for error_report, warning_report, info_report that call > message_report with the right enum. That way you don't have to update any > of the existing code that calls error_report. *Functions*, please, not macros. Macros would bloat the code for no benefit at all.
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h index 3001865896..c2600d2298 100644 --- a/include/qemu/error-report.h +++ b/include/qemu/error-report.h @@ -36,7 +36,9 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); 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 warning_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); +void warning_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2); const char *error_get_progname(void); extern bool enable_timestamp_msg; diff --git a/util/qemu-error.c b/util/qemu-error.c index 1c5e35ecdb..2edd752fec 100644 --- a/util/qemu-error.c +++ b/util/qemu-error.c @@ -203,6 +203,23 @@ void error_vreport(const char *fmt, va_list ap) } /* + * Print a warning message ot the current monitor if we have one, else to + * stderr. This follows similar formating and use cases as error_vreport() + * except for these two differentce: + * - It prefixes the message with 'warning: ' to indicate it is only a + * warning. + * - It does not print the timestamp. + */ +void warning_vreport(const char *fmt, va_list ap) +{ + error_vprintf("warning: ", ap); + + print_loc(); + error_vprintf(fmt, ap); + error_printf("\n"); +} + +/* * Print an error message to current monitor if we have one, else to stderr. * Format arguments like sprintf(). The resulting message should be a * single phrase, with no newline or trailing punctuation. @@ -217,3 +234,18 @@ void error_report(const char *fmt, ...) error_vreport(fmt, ap); va_end(ap); } + +/* + * Print an warning message to current monitor if we have one, else to stderr. + * This follows the same formating and use cases as error_report() + * except it prefixes the message with 'warning: ' to indicate it is only a + * warning. + */ +void warning_report(const char *fmt, ...) +{ + va_list ap; + + va_start(ap, fmt); + warning_vreport(fmt, ap); + va_end(ap); +}
Add a functino which can be used similarly to error_report() execpt to inform the users about warnings instead of errors. The warning print does not include the timestamp and instead will preface the messages with a 'warning: '. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- include/qemu/error-report.h | 2 ++ util/qemu-error.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)