qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror to e


From: Gonglei (Arei)
Subject: Re: [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror to error_report
Date: Thu, 25 Sep 2014 12:53:20 +0000

> Subject: Re: [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror 
> to
> error_report
> 
> On 09/25/2014 03:46 AM, address@hidden wrote:
> > From: Gonglei <address@hidden>
> >
> > Signed-off-by: Gonglei <address@hidden>
> > ---
> >  os-posix.c | 34 ++++++++++++++++++----------------
> >  os-win32.c |  3 ++-
> >  2 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/os-posix.c b/os-posix.c
> > index cb2a7f7..9d5ae70 100644
> > --- a/os-posix.c
> > +++ b/os-posix.c
> > @@ -39,6 +39,7 @@
> >  #include "sysemu/sysemu.h"
> >  #include "net/slirp.h"
> >  #include "qemu-options.h"
> > +#include "qemu/error-report.h"
> >
> >  #ifdef CONFIG_LINUX
> >  #include <sys/prctl.h>
> > @@ -120,11 +121,11 @@ 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");
> 
> This loses the value of errno that perror would have displayed.  Is that
> reduction in error message quality intentional?  

Of course not. :)

> If not, then this is
> not a trivial conversion; if it is, then your commit message should call
> it out.
> 

I agree with you. Actually I missed the usage of perror(), thanks for 
your point. Maybe I should remove the changes about perror() in 
next version.

> > @@ -167,20 +168,20 @@ static void change_process_uid(void)
> >  {
> >      if (user_pwd) {
> >          if (setgid(user_pwd->pw_gid) < 0) {
> > -            fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> > +            error_report("Failed to setgid(%d)\n", user_pwd->pw_gid);
> 
> No trailing \n for error_report, please. (You got it right in most of
> your conversions)
> 

Hmm, yes.

> 
> > @@ -190,11 +191,11 @@ static void change_root(void)
> >  {
> >      if (chroot_dir) {
> >          if (chroot(chroot_dir) < 0) {
> > -            fprintf(stderr, "chroot failed\n");
> > +            error_report("chroot failed");
> >              exit(1);
> >          }
> >          if (chdir("/")) {
> > -            perror("not able to chdir to /");
> > +            error_report("not able to chdir to /");
> 
> Another loss of errno value from perror.
> 
> >              exit(1);
> >          }
> >      }
> > @@ -224,7 +225,7 @@ void os_daemonize(void)
> >              if (len != 1)
> >                  exit(1);
> >              else if (status == 1) {
> > -                fprintf(stderr, "Could not acquire pidfile: %s\n",
> strerror(errno));
> > +                error_report("Could not acquire pidfile: %s",
> strerror(errno));
> 
> This code is broken.  The earlier 'if (len != 1)' fails to print a
> message before exiting (not to mention it violates coding style by
> omitting {}).  Then, if we get inside the 'else if (status == 1)'
> conditional, then we KNOW that read() succeeded, and therefore errno is
> unspecified.  Printing strerror(errno) on a random value is NOT helpful.
> 

Good catch! Will fix those. :)    Thanks for your reviewing! Eric.

Best regards,
-Gonglei

> 
> > @@ -267,7 +268,7 @@ void os_setup_post(void)
> >         exit(1);
> >
> >          if (chdir("/")) {
> > -            perror("not able to chdir to /");
> > +            error_report("not able to chdir to /");
> 
> Another loss of errno reporting.
> 
> >              exit(1);
> >          }
> >     TFR(fd = qemu_open("/dev/null", O_RDWR));
> > @@ -292,10 +293,11 @@ void os_pidfile_error(void)
> >      if (daemonize) {
> >          uint8_t status = 1;
> >          if (write(fds[1], &status, 1) != 1) {
> > -            perror("daemonize. Writing to pipe\n");
> > +            error_report("daemonize. Writing to pipe");
> 
> and another.
> 
> > @@ -338,7 +340,7 @@ int os_mlock(void)
> >
> >      ret = mlockall(MCL_CURRENT | MCL_FUTURE);
> >      if (ret < 0) {
> > -        perror("mlockall");
> > +        error_report("mlockall");
> 
> and another.
> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org


reply via email to

[Prev in Thread] Current Thread [Next in Thread]