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

[Paolo Bonzini]

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
>