* [RFC] util/error-report: Add "error: " prefix for error-level report @ 2024-03-27 11:46 Zhao Liu 2024-03-27 12:36 ` Thomas Huth 0 siblings, 1 reply; 5+ messages in thread From: Zhao Liu @ 2024-03-27 11:46 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, Zhao Liu From: Zhao Liu <zhao1.liu@intel.com> When vreport() was introduced, there was no prefix for error-level (REPORT_TYPE_ERROR) report. The original reason is "To maintain compatibility we don't add anything here" as Alistair said in his RFC v3 series [1]. This was done in the context of inheriting the original error_report() interface without the prefix style. And it was also useful to have a means of error handling, such as exit(), when error occurs, so that the error message - the most serious level - can be noticed by the user. Nowadays, however, error_report() and its variants have a tendency to be "abused": it is used a lot just for the sake of logging something more noticeable than the "warn" or "info" level, in the absence of appropriate error handling logic. But, in the use case above, due to the lack of a prefix, it is in fact less informative to the user than warn_report()/info_report() (with "warn:" or "info: " prfix), which does not match its highest level. Therefore, add "error: " prefix for error-level report. [1]: https://lore.kernel.org/qemu-devel/87r2xuay5h.fsf@dusky.pond.sub.org/#t Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- util/error-report.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/error-report.c b/util/error-report.c index 6e44a5573217..e981c0b032f0 100644 --- a/util/error-report.c +++ b/util/error-report.c @@ -213,6 +213,7 @@ static void vreport(report_type type, const char *fmt, va_list ap) switch (type) { case REPORT_TYPE_ERROR: + error_printf("error: "); break; case REPORT_TYPE_WARNING: error_printf("warning: "); -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC] util/error-report: Add "error: " prefix for error-level report 2024-03-27 11:46 [RFC] util/error-report: Add "error: " prefix for error-level report Zhao Liu @ 2024-03-27 12:36 ` Thomas Huth 2024-03-27 13:02 ` Zhao Liu 2024-03-29 11:10 ` Paolo Bonzini 0 siblings, 2 replies; 5+ messages in thread From: Thomas Huth @ 2024-03-27 12:36 UTC (permalink / raw) To: Zhao Liu, Markus Armbruster; +Cc: qemu-devel, Zhao Liu On 27/03/2024 12.46, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > When vreport() was introduced, there was no prefix for error-level > (REPORT_TYPE_ERROR) report. The original reason is "To maintain > compatibility we don't add anything here" as Alistair said in his > RFC v3 series [1]. > > This was done in the context of inheriting the original error_report() > interface without the prefix style. And it was also useful to have a > means of error handling, such as exit(), when error occurs, so that the > error message - the most serious level - can be noticed by the user. > > Nowadays, however, error_report() and its variants have a tendency to be > "abused": it is used a lot just for the sake of logging something more > noticeable than the "warn" or "info" level, in the absence of > appropriate error handling logic. > > But, in the use case above, due to the lack of a prefix, it is in fact > less informative to the user than warn_report()/info_report() (with > "warn:" or "info: " prfix), which does not match its highest level. > > Therefore, add "error: " prefix for error-level report. > > [1]: https://lore.kernel.org/qemu-devel/87r2xuay5h.fsf@dusky.pond.sub.org/#t > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > util/error-report.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/util/error-report.c b/util/error-report.c > index 6e44a5573217..e981c0b032f0 100644 > --- a/util/error-report.c > +++ b/util/error-report.c > @@ -213,6 +213,7 @@ static void vreport(report_type type, const char *fmt, va_list ap) > > switch (type) { > case REPORT_TYPE_ERROR: > + error_printf("error: "); > break; > case REPORT_TYPE_WARNING: > error_printf("warning: "); Sounds like a good idea to me, but I think you should then also remove the hard-coded "error:" strings in the various error_reports(): $ grep -ri 'error_report.*"error:' migration/block-dirty-bitmap.c: error_report("Error: block device name is not set"); migration/block-dirty-bitmap.c: error_report("Error: Unknown bitmap alias '%s' on node " migration/block-dirty-bitmap.c: error_report("Error: unknown dirty bitmap " migration/block-dirty-bitmap.c: error_report("Error: block device name is not set"); target/i386/kvm/kvm.c: error_report("error: failed to set MSR 0x%" PRIx32 " to 0x%" PRIx64, target/i386/kvm/kvm.c: error_report("error: failed to get MSR 0x%" PRIx32, util/vhost-user-server.c: error_report("Error: too big message request: %d, " accel/hvf/hvf-all.c: error_report("Error: HV_ERROR"); accel/hvf/hvf-all.c: error_report("Error: HV_BUSY"); accel/hvf/hvf-all.c: error_report("Error: HV_BAD_ARGUMENT"); accel/hvf/hvf-all.c: error_report("Error: HV_NO_RESOURCES"); accel/hvf/hvf-all.c: error_report("Error: HV_NO_DEVICE"); accel/hvf/hvf-all.c: error_report("Error: HV_UNSUPPORTED"); accel/hvf/hvf-all.c: error_report("Error: HV_DENIED"); monitor/hmp-cmds.c: error_reportf_err(err, "Error: "); hw/arm/xlnx-zcu102.c: error_report("ERROR: RAM size 0x%" PRIx64 " above max supported of " hw/block/dataplane/xen-block.c: error_report("error: unknown operation (%d)", request->req.operation); hw/block/dataplane/xen-block.c: error_report("error: write req for ro device"); hw/block/dataplane/xen-block.c: error_report("error: nr_segments too big"); hw/block/dataplane/xen-block.c: error_report("error: first > last sector"); hw/block/dataplane/xen-block.c: error_report("error: page crossing"); hw/block/dataplane/xen-block.c: error_report("error: access beyond end of file"); hw/rdma/rdma_backend.c: rdma_error_report("Error: Not a MAD request, skipping"); hw/s390x/s390-skeys.c: error_report("Error: Setting storage keys for pages with unallocated " hw/s390x/s390-skeys.c: error_report("Error: Getting storage keys for pages with unallocated " hw/usb/bus.c: error_report("Error: no usb bus to attach usbdevice %s, " gdbstub/gdbstub.c: error_report("Error: Bad gdb register numbering for '%s', " Thomas ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] util/error-report: Add "error: " prefix for error-level report 2024-03-27 12:36 ` Thomas Huth @ 2024-03-27 13:02 ` Zhao Liu 2024-03-29 11:10 ` Paolo Bonzini 1 sibling, 0 replies; 5+ messages in thread From: Zhao Liu @ 2024-03-27 13:02 UTC (permalink / raw) To: Thomas Huth; +Cc: Markus Armbruster, qemu-devel, Zhao Liu On Wed, Mar 27, 2024 at 01:36:07PM +0100, Thomas Huth wrote: [snip] > Sounds like a good idea to me, but I think you should then also remove > the hard-coded "error:" strings in the various error_reports(): Thanks Thomas! I missed this case, will remove these hard-code prefix first. > $ grep -ri 'error_report.*"error:' > migration/block-dirty-bitmap.c: error_report("Error: block device name is not set"); > migration/block-dirty-bitmap.c: error_report("Error: Unknown bitmap alias '%s' on node " > migration/block-dirty-bitmap.c: error_report("Error: unknown dirty bitmap " > migration/block-dirty-bitmap.c: error_report("Error: block device name is not set"); > target/i386/kvm/kvm.c: error_report("error: failed to set MSR 0x%" PRIx32 " to 0x%" PRIx64, > target/i386/kvm/kvm.c: error_report("error: failed to get MSR 0x%" PRIx32, > util/vhost-user-server.c: error_report("Error: too big message request: %d, " > accel/hvf/hvf-all.c: error_report("Error: HV_ERROR"); > accel/hvf/hvf-all.c: error_report("Error: HV_BUSY"); > accel/hvf/hvf-all.c: error_report("Error: HV_BAD_ARGUMENT"); > accel/hvf/hvf-all.c: error_report("Error: HV_NO_RESOURCES"); > accel/hvf/hvf-all.c: error_report("Error: HV_NO_DEVICE"); > accel/hvf/hvf-all.c: error_report("Error: HV_UNSUPPORTED"); > accel/hvf/hvf-all.c: error_report("Error: HV_DENIED"); > monitor/hmp-cmds.c: error_reportf_err(err, "Error: "); > hw/arm/xlnx-zcu102.c: error_report("ERROR: RAM size 0x%" PRIx64 " above max supported of " > hw/block/dataplane/xen-block.c: error_report("error: unknown operation (%d)", request->req.operation); > hw/block/dataplane/xen-block.c: error_report("error: write req for ro device"); > hw/block/dataplane/xen-block.c: error_report("error: nr_segments too big"); > hw/block/dataplane/xen-block.c: error_report("error: first > last sector"); > hw/block/dataplane/xen-block.c: error_report("error: page crossing"); > hw/block/dataplane/xen-block.c: error_report("error: access beyond end of file"); > hw/rdma/rdma_backend.c: rdma_error_report("Error: Not a MAD request, skipping"); > hw/s390x/s390-skeys.c: error_report("Error: Setting storage keys for pages with unallocated " > hw/s390x/s390-skeys.c: error_report("Error: Getting storage keys for pages with unallocated " > hw/usb/bus.c: error_report("Error: no usb bus to attach usbdevice %s, " > gdbstub/gdbstub.c: error_report("Error: Bad gdb register numbering for '%s', " > > Thomas > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] util/error-report: Add "error: " prefix for error-level report 2024-03-27 12:36 ` Thomas Huth 2024-03-27 13:02 ` Zhao Liu @ 2024-03-29 11:10 ` Paolo Bonzini 2024-03-29 17:15 ` Zhao Liu 1 sibling, 1 reply; 5+ messages in thread From: Paolo Bonzini @ 2024-03-29 11:10 UTC (permalink / raw) To: no-reply; +Cc: qemu-devel, Zhao Liu On Fri, Mar 29, 2024 at 10:37 AM <no-reply@patchew.org> wrote: > > This was done in the context of inheriting the original error_report() > > interface without the prefix style. And it was also useful to have a > > means of error handling, such as exit(), when error occurs, so that the > > error message - the most serious level - can be noticed by the user. > > > > Nowadays, however, error_report() and its variants have a tendency to be > > "abused": it is used a lot just for the sake of logging something more > > noticeable than the "warn" or "info" level, in the absence of > > appropriate error handling logic. Unfortunately, this is the reason why you _cannot_ do what this patch does. For example: error_reportf_err(local_err, "Disconnect client, due to: "); error_report("terminating on signal %d", shutdown_signal); This should not be prepending "error" - it's not an error. error_report_once("%s: detected read error on DMAR slpte " This is a guest error, so "error:" is probably not a good idea (it should use qemu_log_mask). And so on. :( Paolo > > But, in the use case above, due to the lack of a prefix, it is in fact > > less informative to the user than warn_report()/info_report() (with > > "warn:" or "info: " prfix), which does not match its highest level. > > > > Therefore, add "error: " prefix for error-level report. > > > > [1]: https://lore.kernel.org/qemu-devel/87r2xuay5h.fsf@dusky.pond.sub.org/#t > > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > --- > > util/error-report.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/util/error-report.c b/util/error-report.c > > index 6e44a5573217..e981c0b032f0 100644 > > --- a/util/error-report.c > > +++ b/util/error-report.c > > @@ -213,6 +213,7 @@ static void vreport(report_type type, const char *fmt, va_list ap) > > > > switch (type) { > > case REPORT_TYPE_ERROR: > > + error_printf("error: "); > > break; > > case REPORT_TYPE_WARNING: > > error_printf("warning: "); > > Sounds like a good idea to me, but I think you should then also remove > the hard-coded "error:" strings in the various error_reports(): > > $ grep -ri 'error_report.*"error:' > migration/block-dirty-bitmap.c: error_report("Error: block device name is not set"); > migration/block-dirty-bitmap.c: error_report("Error: Unknown bitmap alias '%s' on node " > migration/block-dirty-bitmap.c: error_report("Error: unknown dirty bitmap " > migration/block-dirty-bitmap.c: error_report("Error: block device name is not set"); > target/i386/kvm/kvm.c: error_report("error: failed to set MSR 0x%" PRIx32 " to 0x%" PRIx64, > target/i386/kvm/kvm.c: error_report("error: failed to get MSR 0x%" PRIx32, > util/vhost-user-server.c: error_report("Error: too big message request: %d, " > accel/hvf/hvf-all.c: error_report("Error: HV_ERROR"); > accel/hvf/hvf-all.c: error_report("Error: HV_BUSY"); > accel/hvf/hvf-all.c: error_report("Error: HV_BAD_ARGUMENT"); > accel/hvf/hvf-all.c: error_report("Error: HV_NO_RESOURCES"); > accel/hvf/hvf-all.c: error_report("Error: HV_NO_DEVICE"); > accel/hvf/hvf-all.c: error_report("Error: HV_UNSUPPORTED"); > accel/hvf/hvf-all.c: error_report("Error: HV_DENIED"); > monitor/hmp-cmds.c: error_reportf_err(err, "Error: "); > hw/arm/xlnx-zcu102.c: error_report("ERROR: RAM size 0x%" PRIx64 " above max supported of " > hw/block/dataplane/xen-block.c: error_report("error: unknown operation (%d)", request->req.operation); > hw/block/dataplane/xen-block.c: error_report("error: write req for ro device"); > hw/block/dataplane/xen-block.c: error_report("error: nr_segments too big"); > hw/block/dataplane/xen-block.c: error_report("error: first > last sector"); > hw/block/dataplane/xen-block.c: error_report("error: page crossing"); > hw/block/dataplane/xen-block.c: error_report("error: access beyond end of file"); > hw/rdma/rdma_backend.c: rdma_error_report("Error: Not a MAD request, skipping"); > hw/s390x/s390-skeys.c: error_report("Error: Setting storage keys for pages with unallocated " > hw/s390x/s390-skeys.c: error_report("Error: Getting storage keys for pages with unallocated " > hw/usb/bus.c: error_report("Error: no usb bus to attach usbdevice %s, " > gdbstub/gdbstub.c: error_report("Error: Bad gdb register numbering for '%s', " > > Thomas > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] util/error-report: Add "error: " prefix for error-level report 2024-03-29 11:10 ` Paolo Bonzini @ 2024-03-29 17:15 ` Zhao Liu 0 siblings, 0 replies; 5+ messages in thread From: Zhao Liu @ 2024-03-29 17:15 UTC (permalink / raw) To: Paolo Bonzini; +Cc: no-reply, qemu-devel, Zhao Liu Hi Paolo, On Fri, Mar 29, 2024 at 12:10:17PM +0100, Paolo Bonzini wrote: > Date: Fri, 29 Mar 2024 12:10:17 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [RFC] util/error-report: Add "error: " prefix for error-level > report > > On Fri, Mar 29, 2024 at 10:37 AM <no-reply@patchew.org> wrote: > > > This was done in the context of inheriting the original error_report() > > > interface without the prefix style. And it was also useful to have a > > > means of error handling, such as exit(), when error occurs, so that the > > > error message - the most serious level - can be noticed by the user. > > > > > > Nowadays, however, error_report() and its variants have a tendency to be > > > "abused": it is used a lot just for the sake of logging something more > > > noticeable than the "warn" or "info" level, in the absence of > > > appropriate error handling logic. > > Unfortunately, this is the reason why you _cannot_ do what this patch does. > > For example: > > error_reportf_err(local_err, "Disconnect client, due to: "); > error_report("terminating on signal %d", shutdown_signal); > > This should not be prepending "error" - it's not an error. So I feel these 2 cases maybe should use info_report()? > error_report_once("%s: detected read error on DMAR slpte " > > This is a guest error, so "error:" is probably not a good idea (it > should use qemu_log_mask). Yes, here I can do a cleanup. > And so on. :( error_report() and its variants have 2600+ use cases, and it's impossible to distinguish whether ther're appropriate or not. Thanks for your explanation, I understand this is not workable, since there is too heavy the debt to sort out error_report(). Regards, Zhao ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-29 17:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-27 11:46 [RFC] util/error-report: Add "error: " prefix for error-level report Zhao Liu 2024-03-27 12:36 ` Thomas Huth 2024-03-27 13:02 ` Zhao Liu 2024-03-29 11:10 ` Paolo Bonzini 2024-03-29 17:15 ` Zhao Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.