| Message ID | 20250130134346.1754143-2-clg@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfio: Improve error reporting when MMIO region mapping fails | expand |
Cédric Le Goater <clg@redhat.com> writes: > Depending on the configuration, a passthrough device may produce > recurring DMA mapping errors at runtime and produce a lot of > output. It is useful to report only once. > > Cc: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > include/qapi/error.h | 5 +++++ > util/error.c | 9 +++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 71f8fb2c50eee9a544992d0c05263c9793956fe1..b6ea274882b9788b64d4bb213c3458d7c674a881 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -448,6 +448,11 @@ void error_free_or_abort(Error **errp); > */ > void warn_report_err(Error *err); > > +/* > + * Convenience function to call warn_report_err() once. > + */ > +void warn_report_once_err(Error *err); > + > /* > * Convenience function to error_report() and free @err. > * The report includes hints added with error_append_hint(). > diff --git a/util/error.c b/util/error.c > index e5e247209a9e0796074a9794f5598325f22f8d35..b8a211cccaa609a93091b86316144a0ad0a02662 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -247,6 +247,15 @@ void warn_report_err(Error *err) > error_free(err); > } > > +void warn_report_once_err(Error *err) > +{ > + static bool print_once_; > + if (!print_once_) { > + warn_report_err(err); > + print_once_ = true; > + } > +} Any particular reason for the trailing _ in @print_once_? The first warning suppresses all subsequent warnings, even if they're completely different. PATCH 5 uses this to emit a (rather cryptic) warning about unexpected mappings once. If we later add another use elsewhere, these uses will suppress each other. Is this what we want? The related function warn_report_once_cond() takes the flag as a parameter. Only the calls using the same flag suppress each other. > + > void error_reportf_err(Error *err, const char *fmt, ...) > { > va_list ap;
On 1/30/25 15:25, Markus Armbruster wrote: > Cédric Le Goater <clg@redhat.com> writes: > >> Depending on the configuration, a passthrough device may produce >> recurring DMA mapping errors at runtime and produce a lot of >> output. It is useful to report only once. >> >> Cc: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> include/qapi/error.h | 5 +++++ >> util/error.c | 9 +++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/include/qapi/error.h b/include/qapi/error.h >> index 71f8fb2c50eee9a544992d0c05263c9793956fe1..b6ea274882b9788b64d4bb213c3458d7c674a881 100644 >> --- a/include/qapi/error.h >> +++ b/include/qapi/error.h >> @@ -448,6 +448,11 @@ void error_free_or_abort(Error **errp); >> */ >> void warn_report_err(Error *err); >> >> +/* >> + * Convenience function to call warn_report_err() once. >> + */ >> +void warn_report_once_err(Error *err); >> + >> /* >> * Convenience function to error_report() and free @err. >> * The report includes hints added with error_append_hint(). >> diff --git a/util/error.c b/util/error.c >> index e5e247209a9e0796074a9794f5598325f22f8d35..b8a211cccaa609a93091b86316144a0ad0a02662 100644 >> --- a/util/error.c >> +++ b/util/error.c >> @@ -247,6 +247,15 @@ void warn_report_err(Error *err) >> error_free(err); >> } >> >> +void warn_report_once_err(Error *err) >> +{ >> + static bool print_once_; >> + if (!print_once_) { >> + warn_report_err(err); >> + print_once_ = true; >> + } >> +} > > Any particular reason for the trailing _ in @print_once_?> > The first warning suppresses all subsequent warnings, even if they're > completely different. PATCH 5 uses this to emit a (rather cryptic) > warning about unexpected mappings once. If we later add another use > elsewhere, these uses will suppress each other. Is this what we want? This is copy paste of a static routine that served one purpose in vfio. I wanted to make it common and didn't think enough about the implication. Sorry about that. > The related function warn_report_once_cond() takes the flag as a > parameter. Only the calls using the same flag suppress each other. yep. I wonder if we could use warn_report_once_cond() in a similar macro. Thanks, C. C.
>> The related function warn_report_once_cond() takes the flag as a >> parameter. Only the calls using the same flag suppress each other. > > yep. I wonder if we could use warn_report_once_cond() in a similar > macro. But vfio uses ->hint too which adds complexity. I will keep the custom version I have in vfio for now. Thanks, C.
On Thu, 30 Jan 2025 14:43:38 +0100 Cédric Le Goater <clg@redhat.com> wrote: > Depending on the configuration, a passthrough device may produce > recurring DMA mapping errors at runtime and produce a lot of > output. It is useful to report only once. > > Cc: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > include/qapi/error.h | 5 +++++ > util/error.c | 9 +++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 71f8fb2c50eee9a544992d0c05263c9793956fe1..b6ea274882b9788b64d4bb213c3458d7c674a881 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -448,6 +448,11 @@ void error_free_or_abort(Error **errp); > */ > void warn_report_err(Error *err); > > +/* > + * Convenience function to call warn_report_err() once. > + */ > +void warn_report_once_err(Error *err); > + Turning it into a macro would do what you want: #define warn_report_once_err(err) ({ \ static bool print_once_; \ if (!print_once_) { \ warn_report_err(err); \ print_once_ = true; \ } \ }) So long as we only want once per call site and not once per object, which would pull in something like warn_report_once_cond(). Thanks, Alex > /* > * Convenience function to error_report() and free @err. > * The report includes hints added with error_append_hint(). > diff --git a/util/error.c b/util/error.c > index e5e247209a9e0796074a9794f5598325f22f8d35..b8a211cccaa609a93091b86316144a0ad0a02662 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -247,6 +247,15 @@ void warn_report_err(Error *err) > error_free(err); > } > > +void warn_report_once_err(Error *err) > +{ > + static bool print_once_; > + if (!print_once_) { > + warn_report_err(err); > + print_once_ = true; > + } > +} > + > void error_reportf_err(Error *err, const char *fmt, ...) > { > va_list ap;
On 1/30/25 18:55, Alex Williamson wrote: > On Thu, 30 Jan 2025 14:43:38 +0100 > Cédric Le Goater <clg@redhat.com> wrote: > >> Depending on the configuration, a passthrough device may produce >> recurring DMA mapping errors at runtime and produce a lot of >> output. It is useful to report only once. >> >> Cc: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> include/qapi/error.h | 5 +++++ >> util/error.c | 9 +++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/include/qapi/error.h b/include/qapi/error.h >> index 71f8fb2c50eee9a544992d0c05263c9793956fe1..b6ea274882b9788b64d4bb213c3458d7c674a881 100644 >> --- a/include/qapi/error.h >> +++ b/include/qapi/error.h >> @@ -448,6 +448,11 @@ void error_free_or_abort(Error **errp); >> */ >> void warn_report_err(Error *err); >> >> +/* >> + * Convenience function to call warn_report_err() once. >> + */ >> +void warn_report_once_err(Error *err); >> + > > Turning it into a macro would do what you want: > > #define warn_report_once_err(err) ({ \ > static bool print_once_; \ > if (!print_once_) { \ > warn_report_err(err); \ > print_once_ = true; \ > } \ > }) > > So long as we only want once per call site and not once per object, > which would pull in something like warn_report_once_cond(). Thanks, yeah. I came up with this : /* * TODO: move to util/ */ static bool warn_report_once_err_cond(bool *printed, Error *err) { if (*printed) { error_free(err); return false; } *printed = true; warn_report_err(err); return true; } #define warn_report_once_err(err) \ ({ \ static bool print_once_; \ warn_report_once_err_cond(&print_once_, err); \ }) I don't know where to put it though. It sits in between qapi/error.h and qemu/error-report.h. Thanks, C. > Alex > >> /* >> * Convenience function to error_report() and free @err. >> * The report includes hints added with error_append_hint(). >> diff --git a/util/error.c b/util/error.c >> index e5e247209a9e0796074a9794f5598325f22f8d35..b8a211cccaa609a93091b86316144a0ad0a02662 100644 >> --- a/util/error.c >> +++ b/util/error.c >> @@ -247,6 +247,15 @@ void warn_report_err(Error *err) >> error_free(err); >> } >> >> +void warn_report_once_err(Error *err) >> +{ >> + static bool print_once_; >> + if (!print_once_) { >> + warn_report_err(err); >> + print_once_ = true; >> + } >> +} >> + >> void error_reportf_err(Error *err, const char *fmt, ...) >> { >> va_list ap; >
diff --git a/include/qapi/error.h b/include/qapi/error.h index 71f8fb2c50eee9a544992d0c05263c9793956fe1..b6ea274882b9788b64d4bb213c3458d7c674a881 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -448,6 +448,11 @@ void error_free_or_abort(Error **errp); */ void warn_report_err(Error *err); +/* + * Convenience function to call warn_report_err() once. + */ +void warn_report_once_err(Error *err); + /* * Convenience function to error_report() and free @err. * The report includes hints added with error_append_hint(). diff --git a/util/error.c b/util/error.c index e5e247209a9e0796074a9794f5598325f22f8d35..b8a211cccaa609a93091b86316144a0ad0a02662 100644 --- a/util/error.c +++ b/util/error.c @@ -247,6 +247,15 @@ void warn_report_err(Error *err) error_free(err); } +void warn_report_once_err(Error *err) +{ + static bool print_once_; + if (!print_once_) { + warn_report_err(err); + print_once_ = true; + } +} + void error_reportf_err(Error *err, const char *fmt, ...) { va_list ap;
Depending on the configuration, a passthrough device may produce recurring DMA mapping errors at runtime and produce a lot of output. It is useful to report only once. Cc: Markus Armbruster <armbru@redhat.com> Signed-off-by: Cédric Le Goater <clg@redhat.com> --- include/qapi/error.h | 5 +++++ util/error.c | 9 +++++++++ 2 files changed, 14 insertions(+)