[Qemu-devel] [PATCH 0/4] os: convert fprintf/perror to error_report

[Qemu-devel] [PATCH 0/4] os: convert fprintf/perror to error_report

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Those are trivial patches. Please consider
applying to trivial-next. Thanks!

Gonglei (4):
  os-posix/win32: convert fprintf/perror to error_report
  os-posix: report error message when lock file failed
  osdep: convert fprintf to error_report
  oslib-posix/win32: convert fprintf/perror to error_report

 os-posix.c         | 35 +++++++++++++++++++----------------
 os-win32.c         |  3 ++-
 util/osdep.c       |  8 ++++----
 util/oslib-posix.c |  8 ++++----
 util/oslib-win32.c |  2 +-
 5 files changed, 30 insertions(+), 26 deletions(-)

-- 
1.7.12.4

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

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 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");
         exit(1);
     }
 #else
-    fprintf(stderr, "Change of process name not supported by your OS\n");
+    error_report("Change of process name not supported by your OS");
     exit(1);
 #endif
 }
@@ -145,7 +146,7 @@ void os_parse_cmd_args(int index, const char *optarg)
     case QEMU_OPTION_runas:
         user_pwd = getpwnam(optarg);
         if (!user_pwd) {
-            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
+            error_report("User \"%s\" doesn't exist", optarg);
             exit(1);
         }
         break;
@@ -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);
             exit(1);
         }
         if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
-                    user_pwd->pw_name, user_pwd->pw_gid);
+            error_report("Failed to initgroups(\"%s\", %d)",
+                         user_pwd->pw_name, user_pwd->pw_gid);
             exit(1);
         }
         if (setuid(user_pwd->pw_uid) < 0) {
-            fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
+            error_report("Failed to setuid(%d)", user_pwd->pw_uid);
             exit(1);
         }
         if (setuid(0) != -1) {
-            fprintf(stderr, "Dropping privileges failed\n");
+            error_report("Dropping privileges failed");
             exit(1);
         }
     }
@@ -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 /");
             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));
                 exit(1);
             } else
                 exit(0);
@@ -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 /");
             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");
         }
-    } else
-        fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
+    } else {
+        error_report("Could not acquire pid file: %s", strerror(errno));
+    }
 }
 
 void os_set_line_buffering(void)
@@ -338,7 +340,7 @@ int os_mlock(void)
 
     ret = mlockall(MCL_CURRENT | MCL_FUTURE);
     if (ret < 0) {
-        perror("mlockall");
+        error_report("mlockall");
     }
 
     return ret;
diff --git a/os-win32.c b/os-win32.c
index 5f95caa..28877b3 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -33,6 +33,7 @@
 #include "config-host.h"
 #include "sysemu/sysemu.h"
 #include "qemu-options.h"
+#include "qemu/error-report.h"
 
 /***********************************************************/
 /* Functions missing in mingw */
@@ -106,7 +107,7 @@ void os_parse_cmd_args(int index, const char *optarg)
 
 void os_pidfile_error(void)
 {
-    fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
+    error_report("Could not acquire pid file: %s", strerror(errno));
 }
 
 int qemu_create_pidfile(const char *filename)
-- 
1.7.12.4

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3685 bytes --]

On 09/25/2014 03:46 AM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  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?  If not, then this is
not a trivial conversion; if it is, then your commit message should call
it out.

> @@ -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)


> @@ -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.


> @@ -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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Gonglei (Arei)]

> Subject: Re: [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror to
> error_report
> 
> On 09/25/2014 03:46 AM, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  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

[Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

It will cause that create vm failed When manager
tool is killed forcibly (kill -9 libvirtd_pid),
the file not was unlink, and unlock. It's better
that report the error message for users.

Signed-off-by: Huangweidong <weidong.huang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 os-posix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/os-posix.c b/os-posix.c
index 9d5ae70..89831dc 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
         return -1;
     }
     if (lockf(fd, F_TLOCK, 0) == -1) {
+        error_report("lock file '%s' failed: %s", filename, strerror(errno));
         close(fd);
         return -1;
     }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> It will cause that create vm failed When manager
> tool is killed forcibly (kill -9 libvirtd_pid),
> the file not was unlink, and unlock. It's better
> that report the error message for users.
>
> Signed-off-by: Huangweidong <weidong.huang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  os-posix.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/os-posix.c b/os-posix.c
> index 9d5ae70..89831dc 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
>          return -1;
>      }
>      if (lockf(fd, F_TLOCK, 0) == -1) {
> +        error_report("lock file '%s' failed: %s", filename, strerror(errno));
>          close(fd);
>          return -1;
>      }

Only called from main():

    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
        os_pidfile_error();
        exit(1);
    }

I suspect the error reporting is os_pidfile_error()'s job.  And it
actually does it (POSIX version shown, W32 is simpler):

void os_pidfile_error(void)
{
    if (daemonize) {
        uint8_t status = 1;
        if (write(fds[1], &status, 1) != 1) {
            perror("daemonize. Writing to pipe\n");
        }
    } else
        fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
}

Are you sure your patch makes sense?

Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed

[Gonglei]

Hi,

> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
> lock file failed
> 
> <arei.gonglei@huawei.com> writes:
> 
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > It will cause that create vm failed When manager
> > tool is killed forcibly (kill -9 libvirtd_pid),
> > the file not was unlink, and unlock. It's better
> > that report the error message for users.
> >
> > Signed-off-by: Huangweidong <weidong.huang@huawei.com>
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  os-posix.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/os-posix.c b/os-posix.c
> > index 9d5ae70..89831dc 100644
> > --- a/os-posix.c
> > +++ b/os-posix.c
> > @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
> >          return -1;
> >      }
> >      if (lockf(fd, F_TLOCK, 0) == -1) {
> > +        error_report("lock file '%s' failed: %s", filename, strerror(errno));
> >          close(fd);
> >          return -1;
> >      }
> 
> Only called from main():
> 
Yes, indeed. 

>     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
>         os_pidfile_error();
>         exit(1);
>     }
> 
> I suspect the error reporting is os_pidfile_error()'s job.  And it
> actually does it (POSIX version shown, W32 is simpler):
> 
> void os_pidfile_error(void)
> {
>     if (daemonize) {
>         uint8_t status = 1;
>         if (write(fds[1], &status, 1) != 1) {
>             perror("daemonize. Writing to pipe\n");
>         }
>     } else
>         fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
> }
> 
> Are you sure your patch makes sense?

Yes, I'm sure it make sense. And I had tested, the result is OK as expected.

When daemonize variable is true, the os_pidfile_error() usually don't report an error,
because "if (write(fds[1], &status, 1) != 1)" is always false. On the other
hand, we should assure that we can get the original error message
when lock failed but not other information, such as "Could not acquire pid file...".

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed

[Markus Armbruster]

Gonglei <arei.gonglei@hotmail.com> writes:

> Hi,
>
>> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
>> lock file failed
>> 
>> <arei.gonglei@huawei.com> writes:
>> 
>> > From: Gonglei <arei.gonglei@huawei.com>
>> >
>> > It will cause that create vm failed When manager
>> > tool is killed forcibly (kill -9 libvirtd_pid),
>> > the file not was unlink, and unlock. It's better
>> > that report the error message for users.
>> >
>> > Signed-off-by: Huangweidong <weidong.huang@huawei.com>
>> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> > ---
>> >  os-posix.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/os-posix.c b/os-posix.c
>> > index 9d5ae70..89831dc 100644
>> > --- a/os-posix.c
>> > +++ b/os-posix.c
>> > @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
>> >          return -1;
>> >      }
>> >      if (lockf(fd, F_TLOCK, 0) == -1) {
>> > +        error_report("lock file '%s' failed: %s", filename, strerror(errno));
>> >          close(fd);
>> >          return -1;
>> >      }
>> 
>> Only called from main():
>> 
> Yes, indeed. 
>
>>     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
>>         os_pidfile_error();
>>         exit(1);
>>     }
>> 
>> I suspect the error reporting is os_pidfile_error()'s job.  And it
>> actually does it (POSIX version shown, W32 is simpler):
>> 
>> void os_pidfile_error(void)
>> {
>>     if (daemonize) {
>>         uint8_t status = 1;
>>         if (write(fds[1], &status, 1) != 1) {
>>             perror("daemonize. Writing to pipe\n");
>>         }
>>     } else
>>         fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
>> }
>> 
>> Are you sure your patch makes sense?
>
> Yes, I'm sure it make sense. And I had tested, the result is OK as expected.
>
> When daemonize variable is true, the os_pidfile_error() usually don't
> report an error,
> because "if (write(fds[1], &status, 1) != 1)" is always false. On the other
> hand, we should assure that we can get the original error message
> when lock failed but not other information, such as "Could not acquire
> pid file...".

Even if the new error message makes sense, reporting errors in two
places doesn't.

qemu_create_pidfile() is designed to leave the actual error reporting to
its caller, which delegates it to os_pidfile_error().

If daemonize, os_pidfile_error() signals the failure to the parent
process instead of reporting to stderr.  The parent does the reporting,
in os_daemonize().  If this isn't good enough, you're certainly welcome
to improve it.

Just noticed that commit e5048d1 "os-posix: report error message when
lock file failed" already messed this up: error reporting is spread over
three places: qemu_create_pidfile(), os_pidfile_error() and
os_daemonize().

Please pick one method to report errors: either just in
qemu_create_pidfile(), or in os_pidfile_error() (normal case) and
os_daemonize() (if daemonize).  I don't particularly care which one you
pick, as long as it works with and without -daemonize.

Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed

[Gonglei]

> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
> lock file failed
> 
> Gonglei <arei.gonglei@hotmail.com> writes:
> 
> > Hi,
> >
> >> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
> >> lock file failed
> >>
> >> <arei.gonglei@huawei.com> writes:
> >>
> >> > From: Gonglei <arei.gonglei@huawei.com>
> >> >
> >> > It will cause that create vm failed When manager
> >> > tool is killed forcibly (kill -9 libvirtd_pid),
> >> > the file not was unlink, and unlock. It's better
> >> > that report the error message for users.
> >> >
> >> > Signed-off-by: Huangweidong <weidong.huang@huawei.com>
> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> > ---
> >> >  os-posix.c | 1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> > diff --git a/os-posix.c b/os-posix.c
> >> > index 9d5ae70..89831dc 100644
> >> > --- a/os-posix.c
> >> > +++ b/os-posix.c
> >> > @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
> >> >          return -1;
> >> >      }
> >> >      if (lockf(fd, F_TLOCK, 0) == -1) {
> >> > +        error_report("lock file '%s' failed: %s", filename,
> strerror(errno));
> >> >          close(fd);
> >> >          return -1;
> >> >      }
> >>
> >> Only called from main():
> >>
> > Yes, indeed.
> >
> >>     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
> >>         os_pidfile_error();
> >>         exit(1);
> >>     }
> >>
> >> I suspect the error reporting is os_pidfile_error()'s job.  And it
> >> actually does it (POSIX version shown, W32 is simpler):
> >>
> >> void os_pidfile_error(void)
> >> {
> >>     if (daemonize) {
> >>         uint8_t status = 1;
> >>         if (write(fds[1], &status, 1) != 1) {
> >>             perror("daemonize. Writing to pipe\n");
> >>         }
> >>     } else
> >>         fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
> >> }
> >>
> >> Are you sure your patch makes sense?
> >
> > Yes, I'm sure it make sense. And I had tested, the result is OK as expected.
> >
> > When daemonize variable is true, the os_pidfile_error() usually don't
> > report an error,
> > because "if (write(fds[1], &status, 1) != 1)" is always false. On the other
> > hand, we should assure that we can get the original error message
> > when lock failed but not other information, such as "Could not acquire
> > pid file...".
> 
> Even if the new error message makes sense, reporting errors in two
> places doesn't.
> 
> qemu_create_pidfile() is designed to leave the actual error reporting to
> its caller, which delegates it to os_pidfile_error().
> 
> If daemonize, os_pidfile_error() signals the failure to the parent
> process instead of reporting to stderr.  The parent does the reporting,
> in os_daemonize().  If this isn't good enough, you're certainly welcome
> to improve it.
> 
> Just noticed that commit e5048d1 "os-posix: report error message when
> lock file failed" already messed this up: error reporting is spread over
> three places: qemu_create_pidfile(), os_pidfile_error() and
> os_daemonize().
> 
> Please pick one method to report errors: either just in
> qemu_create_pidfile(), or in os_pidfile_error() (normal case) and
> os_daemonize() (if daemonize).  I don't particularly care which one you
> pick, as long as it works with and without -daemonize.

If we can get the root reason of one error easier, why not?
(I encountered the scenario described in commit message,
I only repeated that issue and eventually got the root reason is locking
pid file failed)

I don't think it is a problem that reporting errors over
two or three places. 

And I often encounter this coding style:

Int funA() {
   ...
   Printf(".....");
   Return -1;
}

Int funcB() {
    If (funA() < 0) {
        Printf("something wrong\n");
        Return -1;
    }
}

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed

[Markus Armbruster]

Gonglei <arei.gonglei@hotmail.com> writes:

>> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
>> lock file failed
>> 
>> Gonglei <arei.gonglei@hotmail.com> writes:
>> 
>> > Hi,
>> >
>> >> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
>> >> lock file failed
>> >>
>> >> <arei.gonglei@huawei.com> writes:
>> >>
>> >> > From: Gonglei <arei.gonglei@huawei.com>
>> >> >
>> >> > It will cause that create vm failed When manager
>> >> > tool is killed forcibly (kill -9 libvirtd_pid),
>> >> > the file not was unlink, and unlock. It's better
>> >> > that report the error message for users.
>> >> >
>> >> > Signed-off-by: Huangweidong <weidong.huang@huawei.com>
>> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> >> > ---
>> >> >  os-posix.c | 1 +
>> >> >  1 file changed, 1 insertion(+)
>> >> >
>> >> > diff --git a/os-posix.c b/os-posix.c
>> >> > index 9d5ae70..89831dc 100644
>> >> > --- a/os-posix.c
>> >> > +++ b/os-posix.c
>> >> > @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
>> >> >          return -1;
>> >> >      }
>> >> >      if (lockf(fd, F_TLOCK, 0) == -1) {
>> >> > +        error_report("lock file '%s' failed: %s", filename,
>> strerror(errno));
>> >> >          close(fd);
>> >> >          return -1;
>> >> >      }
>> >>
>> >> Only called from main():
>> >>
>> > Yes, indeed.
>> >
>> >>     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
>> >>         os_pidfile_error();
>> >>         exit(1);
>> >>     }
>> >>
>> >> I suspect the error reporting is os_pidfile_error()'s job.  And it
>> >> actually does it (POSIX version shown, W32 is simpler):
>> >>
>> >> void os_pidfile_error(void)
>> >> {
>> >>     if (daemonize) {
>> >>         uint8_t status = 1;
>> >>         if (write(fds[1], &status, 1) != 1) {
>> >>             perror("daemonize. Writing to pipe\n");
>> >>         }
>> >>     } else
>> >>         fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
>> >> }
>> >>
>> >> Are you sure your patch makes sense?
>> >
>> > Yes, I'm sure it make sense. And I had tested, the result is OK as expected.
>> >
>> > When daemonize variable is true, the os_pidfile_error() usually don't
>> > report an error,
>> > because "if (write(fds[1], &status, 1) != 1)" is always false. On the other
>> > hand, we should assure that we can get the original error message
>> > when lock failed but not other information, such as "Could not acquire
>> > pid file...".
>> 
>> Even if the new error message makes sense, reporting errors in two
>> places doesn't.
>> 
>> qemu_create_pidfile() is designed to leave the actual error reporting to
>> its caller, which delegates it to os_pidfile_error().
>> 
>> If daemonize, os_pidfile_error() signals the failure to the parent
>> process instead of reporting to stderr.  The parent does the reporting,
>> in os_daemonize().  If this isn't good enough, you're certainly welcome
>> to improve it.
>> 
>> Just noticed that commit e5048d1 "os-posix: report error message when
>> lock file failed" already messed this up: error reporting is spread over
>> three places: qemu_create_pidfile(), os_pidfile_error() and
>> os_daemonize().
>> 
>> Please pick one method to report errors: either just in
>> qemu_create_pidfile(), or in os_pidfile_error() (normal case) and
>> os_daemonize() (if daemonize).  I don't particularly care which one you
>> pick, as long as it works with and without -daemonize.
>
> If we can get the root reason of one error easier, why not?
> (I encountered the scenario described in commit message,
> I only repeated that issue and eventually got the root reason is locking
> pid file failed)
>
> I don't think it is a problem that reporting errors over
> two or three places. 
>
> And I often encounter this coding style:
>
> Int funA() {
>    ...
>    Printf(".....");
>    Return -1;
> }
>
> Int funcB() {
>     If (funA() < 0) {
>         Printf("something wrong\n");
>         Return -1;
>     }
> }

Just because you see this often doesn't make it good code.  It produces
two error messages rather than one.  One of them is usually useless.

Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/4] os-posix: report error message when lock file failed

[Michael Tokarev]

On 09/25/2014 01:46 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> It will cause that create vm failed When manager
> tool is killed forcibly (kill -9 libvirtd_pid),
> the file not was unlink, and unlock. It's better
> that report the error message for users.
> 
> Signed-off-by: Huangweidong <weidong.huang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  os-posix.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/os-posix.c b/os-posix.c
> index 9d5ae70..89831dc 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
>          return -1;
>      }
>      if (lockf(fd, F_TLOCK, 0) == -1) {
> +        error_report("lock file '%s' failed: %s", filename, strerror(errno));
>          close(fd);
>          return -1;
>      }


I think I'll just revert this patch, because indeed, it makes
no sense to do this like that.

Context:

 qemu_create_pidfile() is only created from main(), and there,
 if that function returns failure, os_pidfile_error() function
 is called, to, guess that, report error (which is done differently
 whenever we're daemonizing or not).

 qemu_create_pidfile() function has several error returns, this
 lockf() failure is one of them, there are others (another shown
 in the patch context too).

 So this patch makes whole thing inconsistent at least.

 If we need to show error message when we're daemonizing, it
 looks like we should modify os_pidfile_error() routine to always
 report error and only after that check for daemon mode.  This way
 all errors will be reported the same way.

But I really wonder if we actually need os_pidfile_error() in the
first place, why this can't be done in qemu_create_pidfile().

So, I'm reverting this change now, to be revisited very soon.

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/4] os-posix: report error message when lock file failed

[Gonglei]

On 2014/10/24 15:32, Michael Tokarev wrote:

> On 09/25/2014 01:46 PM, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> It will cause that create vm failed When manager
>> tool is killed forcibly (kill -9 libvirtd_pid),
>> the file not was unlink, and unlock. It's better
>> that report the error message for users.
>>
>> Signed-off-by: Huangweidong <weidong.huang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  os-posix.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/os-posix.c b/os-posix.c
>> index 9d5ae70..89831dc 100644
>> --- a/os-posix.c
>> +++ b/os-posix.c
>> @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename)
>>          return -1;
>>      }
>>      if (lockf(fd, F_TLOCK, 0) == -1) {
>> +        error_report("lock file '%s' failed: %s", filename, strerror(errno));
>>          close(fd);
>>          return -1;
>>      }
> 
> 
> I think I'll just revert this patch, because indeed, it makes
> no sense to do this like that.
> 
> Context:
> 
>  qemu_create_pidfile() is only created from main(), and there,
>  if that function returns failure, os_pidfile_error() function
>  is called, to, guess that, report error (which is done differently
>  whenever we're daemonizing or not).
> 
>  qemu_create_pidfile() function has several error returns, this
>  lockf() failure is one of them, there are others (another shown
>  in the patch context too).
> 

Yes, agree.

>  So this patch makes whole thing inconsistent at least.
> 
>  If we need to show error message when we're daemonizing, it
>  looks like we should modify os_pidfile_error() routine to always
>  report error and only after that check for daemon mode.  This way
>  all errors will be reported the same way.
> 

In os_pidfile_error(), like below...
if (write(fds[1], &status, 1) != 1) {
            perror("daemonize. Writing to pipe\n");
}

will call its child process to report error:

 else if (status == 1) {
                fprintf(stderr, "Could not acquire pidfile\n");
                exit(1);
            }
I just want to make the error message more clear so that
people can find the root reasons of errors as soon as possible. :)

> But I really wonder if we actually need os_pidfile_error() in the
> first place, why this can't be done in qemu_create_pidfile().
> 

> So, I'm reverting this change now, to be revisited very soon.
> 

If you think that's better, I'm fine with it.

Best regards,
-Gonglei

[Qemu-devel] [PATCH 3/4] osdep: convert fprintf to error_report

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 util/osdep.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index b2bd154..7f6e483 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -401,9 +401,9 @@ void fips_set_state(bool requested)
 #endif /* __linux__ */
 
 #ifdef _FIPS_DEBUG
-    fprintf(stderr, "FIPS mode %s (requested %s)\n",
-	    (fips_enabled ? "enabled" : "disabled"),
-	    (requested ? "enabled" : "disabled"));
+    error_report("FIPS mode %s (requested %s)",
+                 (fips_enabled ? "enabled" : "disabled"),
+                 (requested ? "enabled" : "disabled"));
 #endif
 }
 
@@ -428,7 +428,7 @@ int socket_init(void)
     ret = WSAStartup(MAKEWORD(2, 2), &Data);
     if (ret != 0) {
         err = WSAGetLastError();
-        fprintf(stderr, "WSAStartup: %d\n", err);
+        error_report("WSAStartup: %d", err);
         return -1;
     }
     atexit(socket_cleanup);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 3/4] osdep: convert fprintf to error_report

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

On 09/25/2014 03:46 AM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  util/osdep.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/util/osdep.c b/util/osdep.c
> index b2bd154..7f6e483 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -401,9 +401,9 @@ void fips_set_state(bool requested)
>  #endif /* __linux__ */
>  
>  #ifdef _FIPS_DEBUG
> -    fprintf(stderr, "FIPS mode %s (requested %s)\n",
> -	    (fips_enabled ? "enabled" : "disabled"),
> -	    (requested ? "enabled" : "disabled"));
> +    error_report("FIPS mode %s (requested %s)",
> +                 (fips_enabled ? "enabled" : "disabled"),
> +                 (requested ? "enabled" : "disabled"));

Do we really want debugging messages going through error_report()?  This
may be one hunk we don't want.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 3/4] osdep: convert fprintf to error_report

[Gonglei (Arei)]

> Subject: Re: [Qemu-devel] [PATCH 3/4] osdep: convert fprintf to error_report
> 
> On 09/25/2014 03:46 AM, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  util/osdep.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index b2bd154..7f6e483 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -401,9 +401,9 @@ void fips_set_state(bool requested)
> >  #endif /* __linux__ */
> >
> >  #ifdef _FIPS_DEBUG
> > -    fprintf(stderr, "FIPS mode %s (requested %s)\n",
> > -	    (fips_enabled ? "enabled" : "disabled"),
> > -	    (requested ? "enabled" : "disabled"));
> > +    error_report("FIPS mode %s (requested %s)",
> > +                 (fips_enabled ? "enabled" : "disabled"),
> > +                 (requested ? "enabled" : "disabled"));
> 
> Do we really want debugging messages going through error_report()?  This
> may be one hunk we don't want.
> 
Yep, agree.

Best regards,
-Gonglei

> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

[Qemu-devel] [PATCH 4/4] oslib-posix/win32: convert fprintf/perror to error_report

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 util/oslib-posix.c | 8 ++++----
 util/oslib-win32.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 016a047..00310f6 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -88,7 +88,7 @@ int qemu_daemon(int nochdir, int noclose)
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
-        fprintf(stderr, "Failed to allocate memory: %s\n", strerror(errno));
+        error_report("Failed to allocate memory: %s", strerror(errno));
         abort();
     }
     return ptr;
@@ -380,7 +380,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
 
     ret = sigaction(SIGBUS, &act, &oldact);
     if (ret) {
-        perror("os_mem_prealloc: failed to install signal handler");
+        error_report("os_mem_prealloc: failed to install signal handler");
         exit(1);
     }
 
@@ -390,7 +390,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
     pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
 
     if (sigsetjmp(sigjump, 1)) {
-        fprintf(stderr, "os_mem_prealloc: failed to preallocate pages\n");
+        error_report("os_mem_prealloc: failed to preallocate pages");
         exit(1);
     } else {
         int i;
@@ -404,7 +404,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
 
         ret = sigaction(SIGBUS, &oldact, NULL);
         if (ret) {
-            perror("os_mem_prealloc: failed to reinstall signal handler");
+            error_report("os_mem_prealloc: failed to reinstall signal handler");
             exit(1);
         }
 
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index a3eab4a..47a0df7 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -44,7 +44,7 @@
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
-        fprintf(stderr, "Failed to allocate memory: %lu\n", GetLastError());
+        error_report("Failed to allocate memory: %lu", GetLastError());
         abort();
     }
     return ptr;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 4/4] oslib-posix/win32: convert fprintf/perror to error_report

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

On 09/25/2014 03:46 AM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  util/oslib-posix.c | 8 ++++----
>  util/oslib-win32.c | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 

> @@ -380,7 +380,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
>  
>      ret = sigaction(SIGBUS, &act, &oldact);
>      if (ret) {
> -        perror("os_mem_prealloc: failed to install signal handler");
> +        error_report("os_mem_prealloc: failed to install signal handler");
>          exit(1);
>      }
>  
> @@ -390,7 +390,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
>      pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
>  
>      if (sigsetjmp(sigjump, 1)) {
> -        fprintf(stderr, "os_mem_prealloc: failed to preallocate pages\n");
> +        error_report("os_mem_prealloc: failed to preallocate pages");

Another reduction in error message quality.  You'll definitely need to
respin this series, and at this point, I don't know if you can call it
trivial.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 4/4] oslib-posix/win32: convert fprintf/perror to error_report

[Gonglei (Arei)]

> Subject: Re: [Qemu-devel] [PATCH 4/4] oslib-posix/win32: convert fprintf/perror
> to error_report
> 
> On 09/25/2014 03:46 AM, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  util/oslib-posix.c | 8 ++++----
> >  util/oslib-win32.c | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> 
> > @@ -380,7 +380,7 @@ void os_mem_prealloc(int fd, char *area, size_t
> memory)
> >
> >      ret = sigaction(SIGBUS, &act, &oldact);
> >      if (ret) {
> > -        perror("os_mem_prealloc: failed to install signal handler");
> > +        error_report("os_mem_prealloc: failed to install signal handler");
> >          exit(1);
> >      }
> >
> > @@ -390,7 +390,7 @@ void os_mem_prealloc(int fd, char *area, size_t
> memory)
> >      pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
> >
> >      if (sigsetjmp(sigjump, 1)) {
> > -        fprintf(stderr, "os_mem_prealloc: failed to preallocate pages\n");
> > +        error_report("os_mem_prealloc: failed to preallocate pages");
> 
> Another reduction in error message quality.  You'll definitely need to
> respin this series, and at this point, I don't know if you can call it
> trivial.
> 
OK. 

Best regards,
-Gonglei

> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org