| Message ID | 20170713110237.6712-2-lprosek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: > +/* > + * 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 trailing punctuation. The no-LF version allows > + * additional text to be appended with error_printf() or error_vprintf(). > + * Make sure to always close with a newline after all text is printed. > + * Prepends the current location. > + * It's wrong to call this in a QMP monitor. Use error_setg() there. > + */ > +void error_report_nolf(const char *fmt, ...) > +{ > + va_list ap; > + > + va_start(ap, fmt); > + error_vreport_nolf(fmt, ap); > + va_end(ap); > } Each call to this function prepends the timestamp, so it cannot really be used for a sequence of prints in a single line. It's a little ugly but I expected something along the lines of g_strdup_vprintf() in virtio_error(): char *msg; va_start(ap, fmt); msg = g_strdup_vprintf(fmt, ap); va_end(ap); error_report("%s: %s", DEVICE(vdev)->id, msg); g_free(msg); https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf Stefan
On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: >> +/* >> + * 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 trailing punctuation. The no-LF version allows >> + * additional text to be appended with error_printf() or error_vprintf(). >> + * Make sure to always close with a newline after all text is printed. >> + * Prepends the current location. >> + * It's wrong to call this in a QMP monitor. Use error_setg() there. >> + */ >> +void error_report_nolf(const char *fmt, ...) >> +{ >> + va_list ap; >> + >> + va_start(ap, fmt); >> + error_vreport_nolf(fmt, ap); >> + va_end(ap); >> } > > Each call to this function prepends the timestamp, so it cannot really > be used for a sequence of prints in a single line. True, the _nolf means "does not append \n" rather than "can be called repeatedly". You would use error_printf() / error_vprintf() for subsequent prints, as mentioned in the comment above this function. > It's a little ugly but I expected something along the lines of > g_strdup_vprintf() in virtio_error(): > > char *msg; > > va_start(ap, fmt); > msg = g_strdup_vprintf(fmt, ap); > va_end(ap); > > error_report("%s: %s", DEVICE(vdev)->id, msg); > > g_free(msg); > > https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf I wanted to avoid the memory allocation, that's all. Not for performance, obviously, but to increase the chances that it will come through and always look the same if the process is under memory pressure, the heap corrupted or otherwise in a bad state. Both approaches look about the same ugly to me but I can certainly change it to the one you're suggesting. > Stefan
On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote: > On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: > >> +/* > >> + * 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 trailing punctuation. The no-LF version allows > >> + * additional text to be appended with error_printf() or error_vprintf(). > >> + * Make sure to always close with a newline after all text is printed. > >> + * Prepends the current location. > >> + * It's wrong to call this in a QMP monitor. Use error_setg() there. > >> + */ > >> +void error_report_nolf(const char *fmt, ...) > >> +{ > >> + va_list ap; > >> + > >> + va_start(ap, fmt); > >> + error_vreport_nolf(fmt, ap); > >> + va_end(ap); > >> } > > > > Each call to this function prepends the timestamp, so it cannot really > > be used for a sequence of prints in a single line. > > True, the _nolf means "does not append \n" rather than "can be called > repeatedly". You would use error_printf() / error_vprintf() for > subsequent prints, as mentioned in the comment above this function. > > > It's a little ugly but I expected something along the lines of > > g_strdup_vprintf() in virtio_error(): > > > > char *msg; > > > > va_start(ap, fmt); > > msg = g_strdup_vprintf(fmt, ap); > > va_end(ap); > > > > error_report("%s: %s", DEVICE(vdev)->id, msg); > > > > g_free(msg); > > > > https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf > > I wanted to avoid the memory allocation, that's all. Not for > performance, obviously, but to increase the chances that it will come > through and always look the same if the process is under memory > pressure, the heap corrupted or otherwise in a bad state. > > Both approaches look about the same ugly to me but I can certainly > change it to the one you're suggesting. The chance that error_report_nolf() will be used incorrectly is high. Performance isn't a big concern for printing error messages. That's why I suggest a single error_report() call instead. Stefan
On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi wrote: > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: > > +/* > > + * 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 trailing punctuation. The no-LF version allows > > + * additional text to be appended with error_printf() or error_vprintf(). > > + * Make sure to always close with a newline after all text is printed. > > + * Prepends the current location. > > + * It's wrong to call this in a QMP monitor. Use error_setg() there. > > + */ > > +void error_report_nolf(const char *fmt, ...) > > +{ > > + va_list ap; > > + > > + va_start(ap, fmt); > > + error_vreport_nolf(fmt, ap); > > + va_end(ap); > > } > > Each call to this function prepends the timestamp, so it cannot really > be used for a sequence of prints in a single line. > > It's a little ugly but I expected something along the lines of > g_strdup_vprintf() in virtio_error(): > > char *msg; > > va_start(ap, fmt); > msg = g_strdup_vprintf(fmt, ap); > va_end(ap); > > error_report("%s: %s", DEVICE(vdev)->id, msg); > > g_free(msg); You could get the same thing by turning virtio_Error into a macro with a few games. Rename the current method to virtio_error_impl() and then define: #define virtio_error(dev, fmt, ...) \ virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__) Regards, Daniel
Stefan Hajnoczi <stefanha@redhat.com> writes: > On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote: >> On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote: >> > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: >> >> +/* >> >> + * 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 trailing punctuation. The no-LF version allows >> >> + * additional text to be appended with error_printf() or error_vprintf(). >> >> + * Make sure to always close with a newline after all text is printed. >> >> + * Prepends the current location. >> >> + * It's wrong to call this in a QMP monitor. Use error_setg() there. >> >> + */ >> >> +void error_report_nolf(const char *fmt, ...) >> >> +{ >> >> + va_list ap; >> >> + >> >> + va_start(ap, fmt); >> >> + error_vreport_nolf(fmt, ap); >> >> + va_end(ap); >> >> } >> > >> > Each call to this function prepends the timestamp, so it cannot really >> > be used for a sequence of prints in a single line. >> >> True, the _nolf means "does not append \n" rather than "can be called >> repeatedly". You would use error_printf() / error_vprintf() for >> subsequent prints, as mentioned in the comment above this function. >> >> > It's a little ugly but I expected something along the lines of >> > g_strdup_vprintf() in virtio_error(): >> > >> > char *msg; >> > >> > va_start(ap, fmt); >> > msg = g_strdup_vprintf(fmt, ap); >> > va_end(ap); >> > >> > error_report("%s: %s", DEVICE(vdev)->id, msg); >> > >> > g_free(msg); >> > >> > https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf >> >> I wanted to avoid the memory allocation, that's all. Not for >> performance, obviously, but to increase the chances that it will come >> through and always look the same if the process is under memory >> pressure, the heap corrupted or otherwise in a bad state. >> >> Both approaches look about the same ugly to me but I can certainly >> change it to the one you're suggesting. > > The chance that error_report_nolf() will be used incorrectly is high. Concur. When malloc() starts to fail or crash, the process is basically screwed. This should be rare. Keeping error paths simple to improve the chances of a useful error message getting through before the process dies is a laudable idea anyway. But: * When timestamps are enabled, error_vreport() already allocates. * When error messages go to the monitor, monitor_vprintf() already allocates(). We could get rid of both allocations with some effort. Wouldn't affect the interface. Until we do, avoiding another small allocation is unlikely to help. > Performance isn't a big concern for printing error messages. That's why > I suggest a single error_report() call instead.
On Sat, Jul 15, 2017 at 7:50 AM, Markus Armbruster <armbru@redhat.com> wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > >> On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote: >>> On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote: >>> > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: >>> >> +/* >>> >> + * 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 trailing punctuation. The no-LF version allows >>> >> + * additional text to be appended with error_printf() or error_vprintf(). >>> >> + * Make sure to always close with a newline after all text is printed. >>> >> + * Prepends the current location. >>> >> + * It's wrong to call this in a QMP monitor. Use error_setg() there. >>> >> + */ >>> >> +void error_report_nolf(const char *fmt, ...) >>> >> +{ >>> >> + va_list ap; >>> >> + >>> >> + va_start(ap, fmt); >>> >> + error_vreport_nolf(fmt, ap); >>> >> + va_end(ap); >>> >> } >>> > >>> > Each call to this function prepends the timestamp, so it cannot really >>> > be used for a sequence of prints in a single line. >>> >>> True, the _nolf means "does not append \n" rather than "can be called >>> repeatedly". You would use error_printf() / error_vprintf() for >>> subsequent prints, as mentioned in the comment above this function. >>> >>> > It's a little ugly but I expected something along the lines of >>> > g_strdup_vprintf() in virtio_error(): >>> > >>> > char *msg; >>> > >>> > va_start(ap, fmt); >>> > msg = g_strdup_vprintf(fmt, ap); >>> > va_end(ap); >>> > >>> > error_report("%s: %s", DEVICE(vdev)->id, msg); >>> > >>> > g_free(msg); >>> > >>> > https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf >>> >>> I wanted to avoid the memory allocation, that's all. Not for >>> performance, obviously, but to increase the chances that it will come >>> through and always look the same if the process is under memory >>> pressure, the heap corrupted or otherwise in a bad state. >>> >>> Both approaches look about the same ugly to me but I can certainly >>> change it to the one you're suggesting. >> >> The chance that error_report_nolf() will be used incorrectly is high. > > Concur. > > When malloc() starts to fail or crash, the process is basically screwed. > This should be rare. Keeping error paths simple to improve the chances > of a useful error message getting through before the process dies is a > laudable idea anyway. But: > > * When timestamps are enabled, error_vreport() already allocates. > > * When error messages go to the monitor, monitor_vprintf() already > allocates(). > > We could get rid of both allocations with some effort. Wouldn't affect > the interface. Until we do, avoiding another small allocation is > unlikely to help. Makes sense. v3 will have what you guys suggest. Thanks! >> Performance isn't a big concern for printing error messages. That's why >> I suggest a single error_report() call instead.
On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange <berrange@redhat.com> wrote: > On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi wrote: >> On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: >> > +/* >> > + * 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 trailing punctuation. The no-LF version allows >> > + * additional text to be appended with error_printf() or error_vprintf(). >> > + * Make sure to always close with a newline after all text is printed. >> > + * Prepends the current location. >> > + * It's wrong to call this in a QMP monitor. Use error_setg() there. >> > + */ >> > +void error_report_nolf(const char *fmt, ...) >> > +{ >> > + va_list ap; >> > + >> > + va_start(ap, fmt); >> > + error_vreport_nolf(fmt, ap); >> > + va_end(ap); >> > } >> >> Each call to this function prepends the timestamp, so it cannot really >> be used for a sequence of prints in a single line. >> >> It's a little ugly but I expected something along the lines of >> g_strdup_vprintf() in virtio_error(): >> >> char *msg; >> >> va_start(ap, fmt); >> msg = g_strdup_vprintf(fmt, ap); >> va_end(ap); >> >> error_report("%s: %s", DEVICE(vdev)->id, msg); >> >> g_free(msg); > > You could get the same thing by turning virtio_Error into a macro with > a few games. Rename the current method to virtio_error_impl() and then > define: > > #define virtio_error(dev, fmt, ...) \ > virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__) Neat! I think I'll stick with a function though. This doesn't allocate but it adds a little bit of code to each call site which has the potential of slowing down the fast no-error path (I have no data, just the general keeping-the-code-compact-is-good principle). Holler if you disagree! > 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 Mon, Jul 17, 2017 at 08:54:18AM +0200, Ladi Prosek wrote: > On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange > <berrange@redhat.com> wrote: > > On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi wrote: > >> On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: > >> > +/* > >> > + * 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 trailing punctuation. The no-LF version allows > >> > + * additional text to be appended with error_printf() or error_vprintf(). > >> > + * Make sure to always close with a newline after all text is printed. > >> > + * Prepends the current location. > >> > + * It's wrong to call this in a QMP monitor. Use error_setg() there. > >> > + */ > >> > +void error_report_nolf(const char *fmt, ...) > >> > +{ > >> > + va_list ap; > >> > + > >> > + va_start(ap, fmt); > >> > + error_vreport_nolf(fmt, ap); > >> > + va_end(ap); > >> > } > >> > >> Each call to this function prepends the timestamp, so it cannot really > >> be used for a sequence of prints in a single line. > >> > >> It's a little ugly but I expected something along the lines of > >> g_strdup_vprintf() in virtio_error(): > >> > >> char *msg; > >> > >> va_start(ap, fmt); > >> msg = g_strdup_vprintf(fmt, ap); > >> va_end(ap); > >> > >> error_report("%s: %s", DEVICE(vdev)->id, msg); > >> > >> g_free(msg); > > > > You could get the same thing by turning virtio_Error into a macro with > > a few games. Rename the current method to virtio_error_impl() and then > > define: > > > > #define virtio_error(dev, fmt, ...) \ > > virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__) > > Neat! I think I'll stick with a function though. This doesn't allocate > but it adds a little bit of code to each call site which has the > potential of slowing down the fast no-error path (I have no data, just > the general keeping-the-code-compact-is-good principle). Holler if you > disagree! IMHO that would be uneccessary optimization, particular since this is in the error scenario and so is not performance critical to normal operation. Regards, Daniel
On Mon, Jul 17, 2017 at 10:58 AM, Daniel P. Berrange <berrange@redhat.com> wrote: > On Mon, Jul 17, 2017 at 08:54:18AM +0200, Ladi Prosek wrote: >> On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange >> <berrange@redhat.com> wrote: >> > On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi wrote: >> >> On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote: >> >> > +/* >> >> > + * 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 trailing punctuation. The no-LF version allows >> >> > + * additional text to be appended with error_printf() or error_vprintf(). >> >> > + * Make sure to always close with a newline after all text is printed. >> >> > + * Prepends the current location. >> >> > + * It's wrong to call this in a QMP monitor. Use error_setg() there. >> >> > + */ >> >> > +void error_report_nolf(const char *fmt, ...) >> >> > +{ >> >> > + va_list ap; >> >> > + >> >> > + va_start(ap, fmt); >> >> > + error_vreport_nolf(fmt, ap); >> >> > + va_end(ap); >> >> > } >> >> >> >> Each call to this function prepends the timestamp, so it cannot really >> >> be used for a sequence of prints in a single line. >> >> >> >> It's a little ugly but I expected something along the lines of >> >> g_strdup_vprintf() in virtio_error(): >> >> >> >> char *msg; >> >> >> >> va_start(ap, fmt); >> >> msg = g_strdup_vprintf(fmt, ap); >> >> va_end(ap); >> >> >> >> error_report("%s: %s", DEVICE(vdev)->id, msg); >> >> >> >> g_free(msg); >> > >> > You could get the same thing by turning virtio_Error into a macro with >> > a few games. Rename the current method to virtio_error_impl() and then >> > define: >> > >> > #define virtio_error(dev, fmt, ...) \ >> > virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__) >> >> Neat! I think I'll stick with a function though. This doesn't allocate >> but it adds a little bit of code to each call site which has the >> potential of slowing down the fast no-error path (I have no data, just >> the general keeping-the-code-compact-is-good principle). Holler if you >> disagree! > > IMHO that would be uneccessary optimization, particular since this is in > the error scenario and so is not performance critical to normal operation. Yeah, what I mean is that code getting bigger may have negative impact even if it doesn't execute - it takes up space in caches and such. Maybe it's an overkill but some of this common virtio code is pretty low-level and every cache line counts. Actually, I'm tempted to wrap the error conditions in virtio.c in unlikely() so the compiler knows it's not part of normal operation. Thanks! > 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 :|
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 464947f..0e76a73 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2448,8 +2448,9 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...) va_list ap; va_start(ap, fmt); - error_vreport(fmt, ap); + error_vreport_nolf(fmt, ap); va_end(ap); + error_printf("\n"); if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET); diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h index 3001865..cdee84f 100644 --- a/include/qemu/error-report.h +++ b/include/qemu/error-report.h @@ -35,7 +35,8 @@ void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2); 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 error_vreport_nolf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0); +void error_report_nolf(const char *fmt, ...) GCC_FMT_ATTR(1, 2); void error_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 b331f8f..c7fd127 100644 --- a/util/qemu-error.c +++ b/util/qemu-error.c @@ -181,11 +181,13 @@ bool enable_timestamp_msg; /* * Print an error message to current monitor if we have one, else to stderr. * Format arguments like vsprintf(). The resulting message should be - * a single phrase, with no newline or trailing punctuation. - * Prepend the current location and append a newline. + * a single phrase, with no trailing punctuation. The no-LF version allows + * additional text to be appended with error_printf() or error_vprintf(). + * Make sure to always close with a newline after all text is printed. + * Prepends the current location. * It's wrong to call this in a QMP monitor. Use error_setg() there. */ -void error_vreport(const char *fmt, va_list ap) +void error_vreport_nolf(const char *fmt, va_list ap) { GTimeVal tv; gchar *timestr; @@ -199,14 +201,31 @@ void error_vreport(const char *fmt, va_list ap) error_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 trailing punctuation. The no-LF version allows + * additional text to be appended with error_printf() or error_vprintf(). + * Make sure to always close with a newline after all text is printed. + * Prepends the current location. + * It's wrong to call this in a QMP monitor. Use error_setg() there. + */ +void error_report_nolf(const char *fmt, ...) +{ + va_list ap; + + va_start(ap, fmt); + error_vreport_nolf(fmt, ap); + va_end(ap); } /* * 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. - * Prepend the current location and append a newline. + * Prepends the current location and appends a newline. * It's wrong to call this in a QMP monitor. Use error_setg() there. */ void error_report(const char *fmt, ...) @@ -214,6 +233,7 @@ void error_report(const char *fmt, ...) va_list ap; va_start(ap, fmt); - error_vreport(fmt, ap); + error_vreport_nolf(fmt, ap); va_end(ap); + error_printf("\n"); }
Callers of error_report may want to compose the error message out of multiple separate format strings. To save them from always having to combine their strings into one (a process that would likely involve memory allocation which is good to avoid on error paths), new "nolf" variants of error_report and error_vreport are introduced. Signed-off-by: Ladi Prosek <lprosek@redhat.com> --- hw/virtio/virtio.c | 3 ++- include/qemu/error-report.h | 3 ++- util/qemu-error.c | 32 ++++++++++++++++++++++++++------ 3 files changed, 30 insertions(+), 8 deletions(-)