[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fpri
From: |
Eric Blake |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/4] os-posix/win32: convert fprintf/perror to error_report |
Date: |
Thu, 25 Sep 2014 06:33:53 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.0 |
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? 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
signature.asc
Description: OpenPGP digital signature