diff mbox series

[RFC] util/error-report: Add "error: " prefix for error-level report

Message ID 20240327114609.3858483-1-zhao1.liu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [RFC] util/error-report: Add "error: " prefix for error-level report | expand

Commit Message

Zhao Liu March 27, 2024, 11:46 a.m. UTC 
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(+)

Comments

Thomas Huth March 27, 2024, 12:36 p.m. UTC | #1 
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
Zhao Liu March 27, 2024, 1:02 p.m. UTC | #2 
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
> 
>
Paolo Bonzini March 29, 2024, 11:10 a.m. UTC | #3 
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
>
Zhao Liu March 29, 2024, 5:15 p.m. UTC | #4 
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 mbox series

Patch

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: ");