| Message ID | 20191018130716.25438-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Do not use %m in common code to print error messages | expand |
On Fri, Oct 18, 2019 at 03:07:16PM +0200, Thomas Huth wrote: > The %m format specifier is an extension from glibc - and when compiling > QEMU for NetBSD, the compiler correctly complains, e.g.: > > /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': > /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only > allowed in syslog(3) like functions [-Wformat=] > printf("read from sigfd returned %zd: %m\n", len); > ^ > Let's use g_strerror() here instead, which is an easy-to-use wrapper > around the thread-safe strerror_r() function. > > While we're at it, also convert the "printf()" in main-loop.c into > the preferred "error_report()". > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v2: Do not try to g_free() the strings > > hw/misc/tmp421.c | 4 ++-- > util/main-loop.c | 3 ++- > util/systemd.c | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On Fri, Oct 18, 2019 at 03:07:16PM +0200, Thomas Huth wrote: > The %m format specifier is an extension from glibc - and when compiling > QEMU for NetBSD, the compiler correctly complains, e.g.: > > /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': > /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only > allowed in syslog(3) like functions [-Wformat=] > printf("read from sigfd returned %zd: %m\n", len); > ^ > Let's use g_strerror() here instead, which is an easy-to-use wrapper > around the thread-safe strerror_r() function. > > While we're at it, also convert the "printf()" in main-loop.c into > the preferred "error_report()". > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v2: Do not try to g_free() the strings > > hw/misc/tmp421.c | 4 ++-- > util/main-loop.c | 3 ++- > util/systemd.c | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) There are many uses of %m also in hw/vfio/ but that's Linux stuff. Should we change those too or it doesn't matter since it never really compiled on NetBSD? Anyway, this patch LGTM: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thanks, Stefano > > diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c > index 9f044705fa..c0bc150bca 100644 > --- a/hw/misc/tmp421.c > +++ b/hw/misc/tmp421.c > @@ -120,7 +120,7 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name, > int tempid; > > if (sscanf(name, "temperature%d", &tempid) != 1) { > - error_setg(errp, "error reading %s: %m", name); > + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); > return; > } > > @@ -160,7 +160,7 @@ static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name, > } > > if (sscanf(name, "temperature%d", &tempid) != 1) { > - error_setg(errp, "error reading %s: %m", name); > + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); > return; > } > > diff --git a/util/main-loop.c b/util/main-loop.c > index e3eaa55866..eda63fe4e0 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -61,7 +61,8 @@ static void sigfd_handler(void *opaque) > } > > if (len != sizeof(info)) { > - printf("read from sigfd returned %zd: %m\n", len); > + error_report("read from sigfd returned %zd: %s", len, > + g_strerror(errno)); > return; > } > > diff --git a/util/systemd.c b/util/systemd.c > index d22e86c707..1dd0367d9a 100644 > --- a/util/systemd.c > +++ b/util/systemd.c > @@ -60,8 +60,8 @@ unsigned int check_socket_activation(void) > * and we should exit. > */ > error_report("Socket activation failed: " > - "invalid file descriptor fd = %d: %m", > - fd); > + "invalid file descriptor fd = %d: %s", > + fd, g_strerror(errno)); > exit(EXIT_FAILURE); > } > } > -- > 2.18.1 > > --
On 18.10.2019 15:42, Stefano Garzarella wrote: > On Fri, Oct 18, 2019 at 03:07:16PM +0200, Thomas Huth wrote: >> The %m format specifier is an extension from glibc - and when compiling >> QEMU for NetBSD, the compiler correctly complains, e.g.: >> >> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': >> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only >> allowed in syslog(3) like functions [-Wformat=] >> printf("read from sigfd returned %zd: %m\n", len); >> ^ >> Let's use g_strerror() here instead, which is an easy-to-use wrapper >> around the thread-safe strerror_r() function. >> >> While we're at it, also convert the "printf()" in main-loop.c into >> the preferred "error_report()". >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> v2: Do not try to g_free() the strings >> >> hw/misc/tmp421.c | 4 ++-- >> util/main-loop.c | 3 ++- >> util/systemd.c | 4 ++-- >> 3 files changed, 6 insertions(+), 5 deletions(-) > > There are many uses of %m also in hw/vfio/ but that's Linux stuff. > Should we change those too or it doesn't matter since it never really > compiled on NetBSD? > It's a gnu (glibc) extension and linux can use alternative libc implementations. Probably most of them capable to host qemu use %m. > Anyway, this patch LGTM: > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > > Thanks, > Stefano > >> >> diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c >> index 9f044705fa..c0bc150bca 100644 >> --- a/hw/misc/tmp421.c >> +++ b/hw/misc/tmp421.c >> @@ -120,7 +120,7 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name, >> int tempid; >> >> if (sscanf(name, "temperature%d", &tempid) != 1) { >> - error_setg(errp, "error reading %s: %m", name); >> + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); >> return; >> } >> >> @@ -160,7 +160,7 @@ static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name, >> } >> >> if (sscanf(name, "temperature%d", &tempid) != 1) { >> - error_setg(errp, "error reading %s: %m", name); >> + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); >> return; >> } >> >> diff --git a/util/main-loop.c b/util/main-loop.c >> index e3eaa55866..eda63fe4e0 100644 >> --- a/util/main-loop.c >> +++ b/util/main-loop.c >> @@ -61,7 +61,8 @@ static void sigfd_handler(void *opaque) >> } >> >> if (len != sizeof(info)) { >> - printf("read from sigfd returned %zd: %m\n", len); >> + error_report("read from sigfd returned %zd: %s", len, >> + g_strerror(errno)); >> return; >> } >> >> diff --git a/util/systemd.c b/util/systemd.c >> index d22e86c707..1dd0367d9a 100644 >> --- a/util/systemd.c >> +++ b/util/systemd.c >> @@ -60,8 +60,8 @@ unsigned int check_socket_activation(void) >> * and we should exit. >> */ >> error_report("Socket activation failed: " >> - "invalid file descriptor fd = %d: %m", >> - fd); >> + "invalid file descriptor fd = %d: %s", >> + fd, g_strerror(errno)); >> exit(EXIT_FAILURE); >> } >> } >> -- >> 2.18.1 >> >> >
On 18/10/2019 15.49, Kamil Rytarowski wrote: > On 18.10.2019 15:42, Stefano Garzarella wrote: >> On Fri, Oct 18, 2019 at 03:07:16PM +0200, Thomas Huth wrote: >>> The %m format specifier is an extension from glibc - and when compiling >>> QEMU for NetBSD, the compiler correctly complains, e.g.: >>> >>> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': >>> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only >>> allowed in syslog(3) like functions [-Wformat=] >>> printf("read from sigfd returned %zd: %m\n", len); >>> ^ >>> Let's use g_strerror() here instead, which is an easy-to-use wrapper >>> around the thread-safe strerror_r() function. >>> >>> While we're at it, also convert the "printf()" in main-loop.c into >>> the preferred "error_report()". >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> v2: Do not try to g_free() the strings >>> >>> hw/misc/tmp421.c | 4 ++-- >>> util/main-loop.c | 3 ++- >>> util/systemd.c | 4 ++-- >>> 3 files changed, 6 insertions(+), 5 deletions(-) >> >> There are many uses of %m also in hw/vfio/ but that's Linux stuff. >> Should we change those too or it doesn't matter since it never really >> compiled on NetBSD? > > It's a gnu (glibc) extension and linux can use alternative libc > implementations. Probably most of them capable to host qemu use %m. I think I read somewhere that other libcs on Linux also support %m (like musl), but I just can't find that reference anymore. Anyway, we can still fix that later in case someone hits the issue. >> Anyway, this patch LGTM: >> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thanks, Thomas
On Fri, Oct 18, 2019 at 06:01:22PM +0200, Thomas Huth wrote: > On 18/10/2019 15.49, Kamil Rytarowski wrote: > > On 18.10.2019 15:42, Stefano Garzarella wrote: > >> On Fri, Oct 18, 2019 at 03:07:16PM +0200, Thomas Huth wrote: > >>> The %m format specifier is an extension from glibc - and when compiling > >>> QEMU for NetBSD, the compiler correctly complains, e.g.: > >>> > >>> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': > >>> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only > >>> allowed in syslog(3) like functions [-Wformat=] > >>> printf("read from sigfd returned %zd: %m\n", len); > >>> ^ > >>> Let's use g_strerror() here instead, which is an easy-to-use wrapper > >>> around the thread-safe strerror_r() function. > >>> > >>> While we're at it, also convert the "printf()" in main-loop.c into > >>> the preferred "error_report()". > >>> > >>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>> --- > >>> v2: Do not try to g_free() the strings > >>> > >>> hw/misc/tmp421.c | 4 ++-- > >>> util/main-loop.c | 3 ++- > >>> util/systemd.c | 4 ++-- > >>> 3 files changed, 6 insertions(+), 5 deletions(-) > >> > >> There are many uses of %m also in hw/vfio/ but that's Linux stuff. > >> Should we change those too or it doesn't matter since it never really > >> compiled on NetBSD? > > > > It's a gnu (glibc) extension and linux can use alternative libc > > implementations. Probably most of them capable to host qemu use %m. > > I think I read somewhere that other libcs on Linux also support %m (like Good to know! > musl), but I just can't find that reference anymore. Anyway, we can > still fix that later in case someone hits the issue. > Yes, make sense to me. Thanks, Stefano
On 18/10/19 15:07, Thomas Huth wrote: > The %m format specifier is an extension from glibc - and when compiling > QEMU for NetBSD, the compiler correctly complains, e.g.: > > /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': > /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only > allowed in syslog(3) like functions [-Wformat=] > printf("read from sigfd returned %zd: %m\n", len); > ^ > Let's use g_strerror() here instead, which is an easy-to-use wrapper > around the thread-safe strerror_r() function. > > While we're at it, also convert the "printf()" in main-loop.c into > the preferred "error_report()". > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v2: Do not try to g_free() the strings > > hw/misc/tmp421.c | 4 ++-- > util/main-loop.c | 3 ++- > util/systemd.c | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c > index 9f044705fa..c0bc150bca 100644 > --- a/hw/misc/tmp421.c > +++ b/hw/misc/tmp421.c > @@ -120,7 +120,7 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name, > int tempid; > > if (sscanf(name, "temperature%d", &tempid) != 1) { > - error_setg(errp, "error reading %s: %m", name); > + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); > return; > } > > @@ -160,7 +160,7 @@ static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name, > } > > if (sscanf(name, "temperature%d", &tempid) != 1) { > - error_setg(errp, "error reading %s: %m", name); > + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); > return; > } > > diff --git a/util/main-loop.c b/util/main-loop.c > index e3eaa55866..eda63fe4e0 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -61,7 +61,8 @@ static void sigfd_handler(void *opaque) > } > > if (len != sizeof(info)) { > - printf("read from sigfd returned %zd: %m\n", len); > + error_report("read from sigfd returned %zd: %s", len, > + g_strerror(errno)); > return; > } > > diff --git a/util/systemd.c b/util/systemd.c > index d22e86c707..1dd0367d9a 100644 > --- a/util/systemd.c > +++ b/util/systemd.c > @@ -60,8 +60,8 @@ unsigned int check_socket_activation(void) > * and we should exit. > */ > error_report("Socket activation failed: " > - "invalid file descriptor fd = %d: %m", > - fd); > + "invalid file descriptor fd = %d: %s", > + fd, g_strerror(errno)); > exit(EXIT_FAILURE); > } > } > Queued, thanks. Paolo
Le 18/10/2019 à 15:07, Thomas Huth a écrit : > The %m format specifier is an extension from glibc - and when compiling > QEMU for NetBSD, the compiler correctly complains, e.g.: > > /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': > /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only > allowed in syslog(3) like functions [-Wformat=] > printf("read from sigfd returned %zd: %m\n", len); > ^ > Let's use g_strerror() here instead, which is an easy-to-use wrapper > around the thread-safe strerror_r() function. > > While we're at it, also convert the "printf()" in main-loop.c into > the preferred "error_report()". > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v2: Do not try to g_free() the strings > > hw/misc/tmp421.c | 4 ++-- > util/main-loop.c | 3 ++- > util/systemd.c | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c > index 9f044705fa..c0bc150bca 100644 > --- a/hw/misc/tmp421.c > +++ b/hw/misc/tmp421.c > @@ -120,7 +120,7 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name, > int tempid; > > if (sscanf(name, "temperature%d", &tempid) != 1) { > - error_setg(errp, "error reading %s: %m", name); > + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); > return; > } > > @@ -160,7 +160,7 @@ static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name, > } > > if (sscanf(name, "temperature%d", &tempid) != 1) { > - error_setg(errp, "error reading %s: %m", name); > + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); > return; > } > > diff --git a/util/main-loop.c b/util/main-loop.c > index e3eaa55866..eda63fe4e0 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -61,7 +61,8 @@ static void sigfd_handler(void *opaque) > } > > if (len != sizeof(info)) { > - printf("read from sigfd returned %zd: %m\n", len); > + error_report("read from sigfd returned %zd: %s", len, > + g_strerror(errno)); > return; > } > > diff --git a/util/systemd.c b/util/systemd.c > index d22e86c707..1dd0367d9a 100644 > --- a/util/systemd.c > +++ b/util/systemd.c > @@ -60,8 +60,8 @@ unsigned int check_socket_activation(void) > * and we should exit. > */ > error_report("Socket activation failed: " > - "invalid file descriptor fd = %d: %m", > - fd); > + "invalid file descriptor fd = %d: %s", > + fd, g_strerror(errno)); > exit(EXIT_FAILURE); > } > } > Applied to my trivial-patches branch. Thanks, Laurent
Le 21/10/2019 à 18:29, Laurent Vivier a écrit : > Le 18/10/2019 à 15:07, Thomas Huth a écrit : >> The %m format specifier is an extension from glibc - and when compiling >> QEMU for NetBSD, the compiler correctly complains, e.g.: >> >> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': >> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only >> allowed in syslog(3) like functions [-Wformat=] >> printf("read from sigfd returned %zd: %m\n", len); >> ^ >> Let's use g_strerror() here instead, which is an easy-to-use wrapper >> around the thread-safe strerror_r() function. >> >> While we're at it, also convert the "printf()" in main-loop.c into >> the preferred "error_report()". >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> v2: Do not try to g_free() the strings >> >> hw/misc/tmp421.c | 4 ++-- >> util/main-loop.c | 3 ++- >> util/systemd.c | 4 ++-- >> 3 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c >> index 9f044705fa..c0bc150bca 100644 >> --- a/hw/misc/tmp421.c >> +++ b/hw/misc/tmp421.c >> @@ -120,7 +120,7 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name, >> int tempid; >> >> if (sscanf(name, "temperature%d", &tempid) != 1) { >> - error_setg(errp, "error reading %s: %m", name); >> + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); >> return; >> } >> >> @@ -160,7 +160,7 @@ static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name, >> } >> >> if (sscanf(name, "temperature%d", &tempid) != 1) { >> - error_setg(errp, "error reading %s: %m", name); >> + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); >> return; >> } >> >> diff --git a/util/main-loop.c b/util/main-loop.c >> index e3eaa55866..eda63fe4e0 100644 >> --- a/util/main-loop.c >> +++ b/util/main-loop.c >> @@ -61,7 +61,8 @@ static void sigfd_handler(void *opaque) >> } >> >> if (len != sizeof(info)) { >> - printf("read from sigfd returned %zd: %m\n", len); >> + error_report("read from sigfd returned %zd: %s", len, >> + g_strerror(errno)); >> return; >> } >> >> diff --git a/util/systemd.c b/util/systemd.c >> index d22e86c707..1dd0367d9a 100644 >> --- a/util/systemd.c >> +++ b/util/systemd.c >> @@ -60,8 +60,8 @@ unsigned int check_socket_activation(void) >> * and we should exit. >> */ >> error_report("Socket activation failed: " >> - "invalid file descriptor fd = %d: %m", >> - fd); >> + "invalid file descriptor fd = %d: %s", >> + fd, g_strerror(errno)); >> exit(EXIT_FAILURE); >> } >> } >> > > Applied to my trivial-patches branch. As Paolo has already queued it, I remove it from my queue. Thanks, Laurent
diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c index 9f044705fa..c0bc150bca 100644 --- a/hw/misc/tmp421.c +++ b/hw/misc/tmp421.c @@ -120,7 +120,7 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name, int tempid; if (sscanf(name, "temperature%d", &tempid) != 1) { - error_setg(errp, "error reading %s: %m", name); + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); return; } @@ -160,7 +160,7 @@ static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name, } if (sscanf(name, "temperature%d", &tempid) != 1) { - error_setg(errp, "error reading %s: %m", name); + error_setg(errp, "error reading %s: %s", name, g_strerror(errno)); return; } diff --git a/util/main-loop.c b/util/main-loop.c index e3eaa55866..eda63fe4e0 100644 --- a/util/main-loop.c +++ b/util/main-loop.c @@ -61,7 +61,8 @@ static void sigfd_handler(void *opaque) } if (len != sizeof(info)) { - printf("read from sigfd returned %zd: %m\n", len); + error_report("read from sigfd returned %zd: %s", len, + g_strerror(errno)); return; } diff --git a/util/systemd.c b/util/systemd.c index d22e86c707..1dd0367d9a 100644 --- a/util/systemd.c +++ b/util/systemd.c @@ -60,8 +60,8 @@ unsigned int check_socket_activation(void) * and we should exit. */ error_report("Socket activation failed: " - "invalid file descriptor fd = %d: %m", - fd); + "invalid file descriptor fd = %d: %s", + fd, g_strerror(errno)); exit(EXIT_FAILURE); } }
The %m format specifier is an extension from glibc - and when compiling QEMU for NetBSD, the compiler correctly complains, e.g.: /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler': /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only allowed in syslog(3) like functions [-Wformat=] printf("read from sigfd returned %zd: %m\n", len); ^ Let's use g_strerror() here instead, which is an easy-to-use wrapper around the thread-safe strerror_r() function. While we're at it, also convert the "printf()" in main-loop.c into the preferred "error_report()". Signed-off-by: Thomas Huth <thuth@redhat.com> --- v2: Do not try to g_free() the strings hw/misc/tmp421.c | 4 ++-- util/main-loop.c | 3 ++- util/systemd.c | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-)