Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Markus Armbruster <armbru@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");
         }
         return;
     }
@@ -973,11 +975,16 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
 
     if (vfio_blacklist_opt_rom(vdev)) {
         if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
-            error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified non zero value for rombar\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"
+                         " non zero value for rombar\n");
         } else {
-            error_printf("Warning : Rom loading for device at %s has been disabled due to system instability issues. Specify rombar=1 or romfile to force\n",
-                         vdev->vbasedev.name);
+            warn_report("Rom loading for device at %s has been disabled"
+                        " due to system instability issues",
+                        vdev->vbasedev.name);
+            error_printf("Specify rombar=1 or romfile to force\n");
             return;
         }
     }
-- 
2.17.2

On Wed, 17 Apr 2019 21:06:33 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@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,

Alex

>          }
>          return;
>      }
> @@ -973,11 +975,16 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>  
>      if (vfio_blacklist_opt_rom(vdev)) {
>          if (dev->opts && qemu_opt_get(dev->opts, "rombar")) {
> -            error_printf("Warning : Device at %s is known to cause system instability issues during option rom execution. Proceeding anyway since user specified non zero value for rombar\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"
> +                         " non zero value for rombar\n");
>          } else {
> -            error_printf("Warning : Rom loading for device at %s has been disabled due to system instability issues. Specify rombar=1 or romfile to force\n",
> -                         vdev->vbasedev.name);
> +            warn_report("Rom loading for device at %s has been disabled"
> +                        " due to system instability issues",
> +                        vdev->vbasedev.name);
> +            error_printf("Specify rombar=1 or romfile to force\n");
>              return;
>          }
>      }

Alex Williamson <alex.williamson@redhat.com> writes:

> On Wed, 17 Apr 2019 21:06:33 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@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?

On Thu, 18 Apr 2019 08:18:56 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Wed, 17 Apr 2019 21:06:33 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >  
> >> Cc: Alex Williamson <alex.williamson@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@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.williamson@redhat.com>

Alex Williamson <alex.williamson@redhat.com> writes:

> On Thu, 18 Apr 2019 08:18:56 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Alex Williamson <alex.williamson@redhat.com> writes:
>> 
>> > On Wed, 17 Apr 2019 21:06:33 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >  
>> >> Cc: Alex Williamson <alex.williamson@redhat.com>
>> >> Signed-off-by: Markus Armbruster <armbru@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,

Hopefully, presence of good examples, absence of bad examples, and
review will do the trick often enough.

> Acked-by: Alex Williamson <alex.williamson@redhat.com>

Thanks!