Message ID | 20240327114609.3858483-1-zhao1.liu@linux.intel.com |
---|---|
State | New |
Headers | show |
Series | [RFC] util/error-report: Add "error: " prefix for error-level report | expand |
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
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 > >
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 >
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
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: ");