Re: [Qemu-devel] [PATCH v3 07/15] vfio: Report warnings with warn_report(), not error_printf()

[Alex Williamson]

On Wed, 17 Apr 2019 21:06:33 +0200
Markus Armbruster <address@hidden> wrote:

> Cc: Alex Williamson <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  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;
>          }
>      }