| Message ID | 1524156319-11465-16-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/19/2018 01:45 PM, Ian Jackson wrote: > perror() is defined to fprintf(stderr,...). HACKING says > fprintf(stderr,...) is wrong. So perror() is too. > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > CC: Daniel P. Berrange <berrange@redhat.com> > CC: Michael Tokarev <mjt@tls.msk.ru> > CC: Alistair Francis <alistair.francis@xilinx.com> > --- > v7: New patch > --- > os-posix.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/os-posix.c b/os-posix.c > index d4cf466..0108028 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -125,7 +125,7 @@ void os_set_proc_name(const char *s) > /* Could rewrite argv[0] too, but that's a bit more complicated. > This simple way is enough for `top'. */ > if (prctl(PR_SET_NAME, name)) { > - perror("unable to change process name"); > + error_report("unable to change process name: %s", strerror(errno)); > exit(1); > } > #else > @@ -247,7 +247,7 @@ static void change_root(void) > exit(1); > } > if (chdir("/")) { > - perror("not able to chdir to /"); > + error_report("not able to chdir to /: %s", strerror(errno)); > exit(1); > } > } > @@ -309,7 +309,7 @@ void os_setup_post(void) > > if (daemonize) { > if (chdir("/")) { > - perror("not able to chdir to /"); > + error_report("not able to chdir to /: %s", strerror(errno)); > exit(1); > } > TFR(fd = qemu_open("/dev/null", O_RDWR)); > @@ -383,7 +383,7 @@ int os_mlock(void) > > ret = mlockall(MCL_CURRENT | MCL_FUTURE); > if (ret < 0) { > - perror("mlockall"); > + error_report("mlockall: %s", strerror(errno)); > } > > return ret; Thinking loudly, maybe we can refactor as error_report_errno(const char *desc)... Anyway: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report"):
> On 04/19/2018 01:45 PM, Ian Jackson wrote:
> > - perror("mlockall");
> > + error_report("mlockall: %s", strerror(errno));
> > }
> >
> > return ret;
>
> Thinking loudly, maybe we can refactor as error_report_errno(const char
> *desc)...
git-grep 'error_report.*errno' shows a lot of call sites that do
something more exciting than const char *desc would support.
I think the right approach would be
- static void vreport(report_type type, const char *fmt, va_list ap)
+ static void vreport(report_type type, int errnoval, const char *fmt, va_list ap)
...
+ if (errnoval >= 0) {
+ error_printf(": %s", strerror(errnoval);
+ }
and then add both
error_report_errno
error_vreport_errno
with the obvious semantics.
Ian.
On Tue, Apr 24, 2018 at 03:53:48PM +0100, Ian Jackson wrote: > Philippe Mathieu-Daudé writes ("Re: [Qemu-devel] [PATCH 15/16] os-posix: cleanup: Replace perror with error_report"): > > On 04/19/2018 01:45 PM, Ian Jackson wrote: > > > - perror("mlockall"); > > > + error_report("mlockall: %s", strerror(errno)); > > > } > > > > > > return ret; > > > > Thinking loudly, maybe we can refactor as error_report_errno(const char > > *desc)... > > git-grep 'error_report.*errno' shows a lot of call sites that do > something more exciting than const char *desc would support. > > I think the right approach would be > > - static void vreport(report_type type, const char *fmt, va_list ap) > + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap) > ... > + if (errnoval >= 0) { > + error_printf(": %s", strerror(errnoval); > + } > > and then add both > error_report_errno > error_vreport_errno > with the obvious semantics. That would be nice, because then we can make these two functions actually use strerror_r() instead of strerror(), for thread safety on all platforms. Regards, Daniel
On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote: >> - static void vreport(report_type type, const char *fmt, va_list ap) >> + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap) >> ... >> + if (errnoval >= 0) { >> + error_printf(": %s", strerror(errnoval); >> + } >> >> and then add both >> error_report_errno >> error_vreport_errno >> with the obvious semantics. > > That would be nice, because then we can make these two functions actually > use strerror_r() instead of strerror(), for thread safety on all platforms. Except that strerror_r() is a bear to use portably, given that glibc's default declaration differs from the POSIX requirement (you can force glibc to give you the POSIX version, but doing so causes you to lose access to many other useful extensions). It's rather telling that 'git grep strerror_r' currently comes up empty.
On 04/24/2018 10:40 AM, Eric Blake wrote: > On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote: > >>> - static void vreport(report_type type, const char *fmt, va_list ap) >>> + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap) >>> ... >>> + if (errnoval >= 0) { >>> + error_printf(": %s", strerror(errnoval); >>> + } >>> >>> and then add both >>> error_report_errno >>> error_vreport_errno >>> with the obvious semantics. >> >> That would be nice, because then we can make these two functions actually >> use strerror_r() instead of strerror(), for thread safety on all platforms. > > Except that strerror_r() is a bear to use portably, given that glibc's > default declaration differs from the POSIX requirement (you can force > glibc to give you the POSIX version, but doing so causes you to lose > access to many other useful extensions). It's rather telling that 'git > grep strerror_r' currently comes up empty. That said, glib's g_strerror() may be suitable for this purpose, although we are currently using it only in tests/ivshmem-test.c.
On Tue, Apr 24, 2018 at 10:43:09AM -0500, Eric Blake wrote: > On 04/24/2018 10:40 AM, Eric Blake wrote: > > On 04/24/2018 10:18 AM, Daniel P. Berrangé wrote: > > > >>> - static void vreport(report_type type, const char *fmt, va_list ap) > >>> + static void vreport(report_type type, int errnoval, const char *fmt, va_list ap) > >>> ... > >>> + if (errnoval >= 0) { > >>> + error_printf(": %s", strerror(errnoval); > >>> + } > >>> > >>> and then add both > >>> error_report_errno > >>> error_vreport_errno > >>> with the obvious semantics. > >> > >> That would be nice, because then we can make these two functions actually > >> use strerror_r() instead of strerror(), for thread safety on all platforms. > > > > Except that strerror_r() is a bear to use portably, given that glibc's > > default declaration differs from the POSIX requirement (you can force > > glibc to give you the POSIX version, but doing so causes you to lose > > access to many other useful extensions). It's rather telling that 'git > > grep strerror_r' currently comes up empty. > > That said, glib's g_strerror() may be suitable for this purpose, > although we are currently using it only in tests/ivshmem-test.c. Yes, that uses strerror_r internally, or strerror_s on Windows: https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gstrfuncs.c#L1236 Regards, Daniel
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 04/19/2018 01:45 PM, Ian Jackson wrote: >> perror() is defined to fprintf(stderr,...). HACKING says >> fprintf(stderr,...) is wrong. So perror() is too. >> >> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> >> CC: Paolo Bonzini <pbonzini@redhat.com> >> CC: Markus Armbruster <armbru@redhat.com> >> CC: Daniel P. Berrange <berrange@redhat.com> >> CC: Michael Tokarev <mjt@tls.msk.ru> >> CC: Alistair Francis <alistair.francis@xilinx.com> >> --- >> v7: New patch >> --- >> os-posix.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/os-posix.c b/os-posix.c >> index d4cf466..0108028 100644 >> --- a/os-posix.c >> +++ b/os-posix.c >> @@ -125,7 +125,7 @@ void os_set_proc_name(const char *s) >> /* Could rewrite argv[0] too, but that's a bit more complicated. >> This simple way is enough for `top'. */ >> if (prctl(PR_SET_NAME, name)) { >> - perror("unable to change process name"); >> + error_report("unable to change process name: %s", strerror(errno)); >> exit(1); >> } >> #else >> @@ -247,7 +247,7 @@ static void change_root(void) >> exit(1); >> } >> if (chdir("/")) { >> - perror("not able to chdir to /"); >> + error_report("not able to chdir to /: %s", strerror(errno)); >> exit(1); >> } >> } >> @@ -309,7 +309,7 @@ void os_setup_post(void) >> >> if (daemonize) { >> if (chdir("/")) { >> - perror("not able to chdir to /"); >> + error_report("not able to chdir to /: %s", strerror(errno)); >> exit(1); >> } >> TFR(fd = qemu_open("/dev/null", O_RDWR)); >> @@ -383,7 +383,7 @@ int os_mlock(void) >> >> ret = mlockall(MCL_CURRENT | MCL_FUTURE); >> if (ret < 0) { >> - perror("mlockall"); >> + error_report("mlockall: %s", strerror(errno)); >> } >> >> return ret; > > Thinking loudly, maybe we can refactor as error_report_errno(const char > *desc)... Maybe. If you want to try, make it a separate series not blocking this one. > Anyway: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/os-posix.c b/os-posix.c index d4cf466..0108028 100644 --- a/os-posix.c +++ b/os-posix.c @@ -125,7 +125,7 @@ void os_set_proc_name(const char *s) /* Could rewrite argv[0] too, but that's a bit more complicated. This simple way is enough for `top'. */ if (prctl(PR_SET_NAME, name)) { - perror("unable to change process name"); + error_report("unable to change process name: %s", strerror(errno)); exit(1); } #else @@ -247,7 +247,7 @@ static void change_root(void) exit(1); } if (chdir("/")) { - perror("not able to chdir to /"); + error_report("not able to chdir to /: %s", strerror(errno)); exit(1); } } @@ -309,7 +309,7 @@ void os_setup_post(void) if (daemonize) { if (chdir("/")) { - perror("not able to chdir to /"); + error_report("not able to chdir to /: %s", strerror(errno)); exit(1); } TFR(fd = qemu_open("/dev/null", O_RDWR)); @@ -383,7 +383,7 @@ int os_mlock(void) ret = mlockall(MCL_CURRENT | MCL_FUTURE); if (ret < 0) { - perror("mlockall"); + error_report("mlockall: %s", strerror(errno)); } return ret;
perror() is defined to fprintf(stderr,...). HACKING says fprintf(stderr,...) is wrong. So perror() is too. Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Markus Armbruster <armbru@redhat.com> CC: Daniel P. Berrange <berrange@redhat.com> CC: Michael Tokarev <mjt@tls.msk.ru> CC: Alistair Francis <alistair.francis@xilinx.com> --- v7: New patch --- os-posix.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)