| Message ID | 20130905222853.4097.42724.stgit@bling.home |
|---|---|
| State | New |
| Headers | show |
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.
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
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...)
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),
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(-)