[Qemu-devel] [PATCH] vfio-pci: Cleanup error_reports

[Qemu-devel] [PATCH] vfio-pci: Cleanup error_reports

[Alex Williamson]

Remove carriage returns and tweak formatting for error_reports.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/misc/vfio.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 730dec5..a73e7f5 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -3055,13 +3055,15 @@ static int vfio_get_device(VFIOGroup *group, const char *name, VFIODevice *vdev)
     ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
     if (ret) {
         /* This can fail for an old kernel or legacy PCI dev */
-        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure ret=%d\n", ret);
+        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure: %m\n");
         ret = 0;
     } else if (irq_info.count == 1) {
         vdev->pci_aer = true;
     } else {
-        error_report("vfio: Warning: "
-                     "Could not enable error recovery for the device\n");
+        error_report("vfio: %04x:%02x:%02x.%x "
+                     "Could not enable error recovery for the device",
+                     vdev->host.domain, vdev->host.bus, vdev->host.slot,
+                     vdev->host.function);
     }
 
 error:
@@ -3102,11 +3104,10 @@ static void vfio_err_notifier_handler(void *opaque)
      * guest to contain the error.
      */
 
-    error_report("%s (%04x:%02x:%02x.%x)"
-        "Unrecoverable error detected...\n"
-        "Please collect any data possible and then kill the guest",
-        __func__, vdev->host.domain, vdev->host.bus,
-        vdev->host.slot, vdev->host.function);
+    error_report("%s(%04x:%02x:%02x.%x) Unrecoverable error detected.  "
+                 "Please collect any data possible and then kill the guest",
+                 __func__, vdev->host.domain, vdev->host.bus,
+                 vdev->host.slot, vdev->host.function);
 
     vm_stop(RUN_STATE_IO_ERROR);
 }
@@ -3129,8 +3130,7 @@ static void vfio_register_err_notifier(VFIODevice *vdev)
     }
 
     if (event_notifier_init(&vdev->err_notifier, 0)) {
-        error_report("vfio: Warning: "
-                     "Unable to init event notifier for error detection\n");
+        error_report("vfio: Unable to init event notifier for error detection");
         vdev->pci_aer = false;
         return;
     }
@@ -3151,7 +3151,7 @@ static void vfio_register_err_notifier(VFIODevice *vdev)
 
     ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
     if (ret) {
-        error_report("vfio: Failed to set up error notification\n");
+        error_report("vfio: Failed to set up error notification");
         qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
         event_notifier_cleanup(&vdev->err_notifier);
         vdev->pci_aer = false;
@@ -3184,7 +3184,7 @@ static void vfio_unregister_err_notifier(VFIODevice *vdev)
 
     ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
     if (ret) {
-        error_report("vfio: Failed to de-assign error fd: %d\n", ret);
+        error_report("vfio: Failed to de-assign error fd: %m");
     }
     g_free(irq_set);
     qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),

Re: [Qemu-devel] [PATCH] vfio-pci: Cleanup error_reports

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On 09/05/2013 04:29 PM, Alex Williamson wrote:
> Remove carriage returns and tweak formatting for error_reports.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  hw/misc/vfio.c |   24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 730dec5..a73e7f5 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -3055,13 +3055,15 @@ static int vfio_get_device(VFIOGroup *group, const char *name, VFIODevice *vdev)
>      ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
>      if (ret) {
>          /* This can fail for an old kernel or legacy PCI dev */
> -        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure ret=%d\n", ret);
> +        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure: %m\n");

%m is a glibc extension, and does not exist on all platforms.  You
should probably not make this change.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] vfio-pci: Cleanup error_reports

[Alex Williamson]

On Thu, 2013-09-05 at 16:37 -0600, Eric Blake wrote:
> On 09/05/2013 04:29 PM, Alex Williamson wrote:
> > Remove carriage returns and tweak formatting for error_reports.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  hw/misc/vfio.c |   24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> > index 730dec5..a73e7f5 100644
> > --- a/hw/misc/vfio.c
> > +++ b/hw/misc/vfio.c
> > @@ -3055,13 +3055,15 @@ static int vfio_get_device(VFIOGroup *group, const char *name, VFIODevice *vdev)
> >      ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> >      if (ret) {
> >          /* This can fail for an old kernel or legacy PCI dev */
> > -        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure ret=%d\n", ret);
> > +        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure: %m\n");
> 
> %m is a glibc extension, and does not exist on all platforms.  You
> should probably not make this change.

It's not the only instance of %m in this file and, IIRC, was previously
suggested by Anthony.  Are we banning %m now?  Thanks,

Alex

Re: [Qemu-devel] [PATCH] vfio-pci: Cleanup error_reports

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On 09/05/2013 04:41 PM, Alex Williamson wrote:

>>>          /* This can fail for an old kernel or legacy PCI dev */
>>> -        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure ret=%d\n", ret);
>>> +        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure: %m\n");
>>
>> %m is a glibc extension, and does not exist on all platforms.  You
>> should probably not make this change.
> 
> It's not the only instance of %m in this file and, IIRC, was previously
> suggested by Anthony.  Are we banning %m now?  Thanks,

This particular usage is protected by DPRINTF, so as long as DPRINTF is
disabled, it won't break compilation on non-glibc systems.  And in
thinking about it, this _particular_ file is Linux-only (vfio is a Linux
kernel concept not available on other OS), so for _this_ file, you are
assured that %m will always be interpreted by glibc (or its derivatives)
and thus have defined extension behavior.  So I retract any complaint
about this patch.

But I suspect that if we start encouraging the use of %m, we will
eventually proliferate it into other files that are not Linux-only, and
then run into compilers that warn about unknown format specifiers for a
given platform (such as on BSD systems), or worse, silent compilation
but then output that triggers undefined behavior if the printf is ever
actually used at runtime (typically you'd get a literal %m instead of
the intended strerror() text which merely makes the trace less useful,
but undefined behavior also permits a libc that abort()s on seeing %m...)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]