On Thu, 18 Apr 2019 08:18:56 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Alex Williamson <alex.william...@redhat.com> writes:
>
> > On Wed, 17 Apr 2019 21:06:33 +0200
> > Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> Cc: Alex Williamson <alex.william...@redhat.com>
> >> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> >> ---
> >> hw/vfio/pci.c | 19 +++++++++++++------
> >> 1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index 504019c458..0142819ea6 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -947,8 +947,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
> >> if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> >> /* Since pci handles romfile, just print a message and return */
> >> if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
> >> - error_printf("Warning : Device at %s is known to cause system
> >> instability issues during option rom execution. Proceeding anyway since
> >> user specified romfile\n",
> >> - vdev->vbasedev.name);
> >> + warn_report("Device at %s is known to cause system
> >> instability"
> >> + " issues during option rom execution",
> >> + vdev->vbasedev.name);
> >> + error_printf("Proceeding anyway since user specified
> >> romfile\n");
> >
> > I'm confused, the original warning is "this device is know to have
> > issues, proceeding because you asked me to". Are we categorizing the
> > first half as a warning and the latter as random uncategorized error
> > spew? Did an automated script chunk it this way because of the period
> > and strict application of the "single phrase" specification of
> > warn_report()? If this is the recommended semantics, I'm not sure how
> > I'd know to generate this myself for similar situations. Should we
> > instead try to express this in something acceptable as a single
> > phrase? Thanks,
>
> This is an instance of the following error reporting pattern:
>
> concise error / warning message (one line, single phrase)
> additional information (free format)
>
> We use error_report() / warn_report() for the former (this adds
> decorations to the message), and error_printf() for the latter.
>
> "git-grep -w error_printf" will lead you to other instances. Recommend
> to look with this series applied, because it removes misuses of
> error_printf().
>
> Particularly relevant instances are error_report_err() and
> warn_report_err(): these print the error proper with error_report() /
> warn_report_err(), and the hint, if any, with error_printf().
>
> Clearer now?
I can't guarantee that I'd be able to reproduce these sorts of
semantics without prompting, but yes, there does seem to be some method
to the madness ;) Thanks,
Acked-by: Alex Williamson <alex.william...@redhat.com>
