diff mbox series

[15/16] os-posix: cleanup: Replace perror with error_report

Message ID 1524156319-11465-16-git-send-email-ian.jackson@eu.citrix.com
State New
Headers show
Series xen: xen-domid-restrict improvements | expand

Commit Message

Ian Jackson April 19, 2018, 4:45 p.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé April 19, 2018, 8:31 p.m. UTC | #1 
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>
Ian Jackson April 24, 2018, 2:53 p.m. UTC | #2 
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.
Daniel P. Berrangé April 24, 2018, 3:18 p.m. UTC | #3 
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
Eric Blake April 24, 2018, 3:40 p.m. UTC | #4 
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.
Eric Blake April 24, 2018, 3:43 p.m. UTC | #5 
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.
Daniel P. Berrangé April 24, 2018, 3:54 p.m. UTC | #6 
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
Markus Armbruster April 24, 2018, 4:20 p.m. UTC | #7 
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 mbox series

Patch

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;