| Message ID | 1276162951-842-15-git-send-email-Jes.Sorensen@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 2010-06-10 at 05:42 -0400, Jes.Sorensen@redhat.com wrote: > diff --git a/os-posix.c b/os-posix.c > index 6417d16..1672e06 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -160,6 +162,9 @@ void os_parse_cmd_args(int index, const char *optarg) > case QEMU_OPTION_chroot: > chroot_dir = optarg; > break; > + case QEMU_OPTION_daemonize: > + daemonize = 1; > + break; > } > return; > } This move broke the -daemonize option for us. We are using the qemu-kvm tree. The issue is that the QEMU_OPTION_* enumeration between vl.c and os-posix.c is out of sync. In our case MAP_POPULATE is defined in vl.c but is not in os-posix.c. This excludes the option -mem-prealloc in os-posix.c, see qemu-options.def for the ifdef statement. All subsequent options are off by one in comparison to vl.c. Just including sys/mman.h in os-posix.c fixes the issue for me. But I'm not sure if there is a more generic fix to that problem.
On 06/25/10 18:41, Frank Arnold wrote: > On Thu, 2010-06-10 at 05:42 -0400, Jes.Sorensen@redhat.com wrote: >> diff --git a/os-posix.c b/os-posix.c >> index 6417d16..1672e06 100644 >> --- a/os-posix.c >> +++ b/os-posix.c >> @@ -160,6 +162,9 @@ void os_parse_cmd_args(int index, const char *optarg) >> case QEMU_OPTION_chroot: >> chroot_dir = optarg; >> break; >> + case QEMU_OPTION_daemonize: >> + daemonize = 1; >> + break; >> } >> return; >> } > > This move broke the -daemonize option for us. We are using the qemu-kvm > tree. > > The issue is that the QEMU_OPTION_* enumeration between vl.c and > os-posix.c is out of sync. In our case MAP_POPULATE is defined in vl.c > but is not in os-posix.c. This excludes the option -mem-prealloc in > os-posix.c, see qemu-options.def for the ifdef statement. All subsequent > options are off by one in comparison to vl.c. > > Just including sys/mman.h in os-posix.c fixes the issue for me. But I'm > not sure if there is a more generic fix to that problem. Thanks for the update. What do you mean that it changes the numbering, do you get a compile time error or are you saying that it is the order of parsing the options that change? Are you building on Linux or another OS? Cheers, Jes
On Fri, 2010-06-25 at 13:02 -0400, Jes Sorensen wrote: > On 06/25/10 18:41, Frank Arnold wrote: > > On Thu, 2010-06-10 at 05:42 -0400, Jes.Sorensen@redhat.com wrote: > >> diff --git a/os-posix.c b/os-posix.c > >> index 6417d16..1672e06 100644 > >> --- a/os-posix.c > >> +++ b/os-posix.c > >> @@ -160,6 +162,9 @@ void os_parse_cmd_args(int index, const char *optarg) > >> case QEMU_OPTION_chroot: > >> chroot_dir = optarg; > >> break; > >> + case QEMU_OPTION_daemonize: > >> + daemonize = 1; > >> + break; > >> } > >> return; > >> } > > > > This move broke the -daemonize option for us. We are using the qemu-kvm > > tree. > > > > The issue is that the QEMU_OPTION_* enumeration between vl.c and > > os-posix.c is out of sync. In our case MAP_POPULATE is defined in vl.c > > but is not in os-posix.c. This excludes the option -mem-prealloc in > > os-posix.c, see qemu-options.def for the ifdef statement. All subsequent > > options are off by one in comparison to vl.c. > > > > Just including sys/mman.h in os-posix.c fixes the issue for me. But I'm > > not sure if there is a more generic fix to that problem. > > Thanks for the update. What do you mean that it changes the numbering, > do you get a compile time error or are you saying that it is the order > of parsing the options that change? > > Are you building on Linux or another OS? We are doing KVM testing, so it is Linux. What I did is putting lines like this somewhere into vl.c and os-posix.c: fprintf(stderr, "os: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); fprintf(stderr, "vl: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); Resulting in the following output on stderr: os: QEMU_OPTION_daemonize: 85 vl: QEMU_OPTION_daemonize: 86 No compile time errors. The preprocessing of qemu-options.h is done separately for both files. This results in a missing option definition for os-posix.c and discrepancy in the option enumeration.
On Fri, 2010-06-25 at 19:34 +0200, Frank Arnold wrote: > On Fri, 2010-06-25 at 13:02 -0400, Jes Sorensen wrote: > > On 06/25/10 18:41, Frank Arnold wrote: > > > On Thu, 2010-06-10 at 05:42 -0400, Jes.Sorensen@redhat.com wrote: > > >> diff --git a/os-posix.c b/os-posix.c > > >> index 6417d16..1672e06 100644 > > >> --- a/os-posix.c > > >> +++ b/os-posix.c > > >> @@ -160,6 +162,9 @@ void os_parse_cmd_args(int index, const char *optarg) > > >> case QEMU_OPTION_chroot: > > >> chroot_dir = optarg; > > >> break; > > >> + case QEMU_OPTION_daemonize: > > >> + daemonize = 1; > > >> + break; > > >> } > > >> return; > > >> } > > > > > > This move broke the -daemonize option for us. We are using the qemu-kvm > > > tree. > > > > > > The issue is that the QEMU_OPTION_* enumeration between vl.c and > > > os-posix.c is out of sync. In our case MAP_POPULATE is defined in vl.c > > > but is not in os-posix.c. This excludes the option -mem-prealloc in > > > os-posix.c, see qemu-options.def for the ifdef statement. All subsequent > > > options are off by one in comparison to vl.c. > > > > > > Just including sys/mman.h in os-posix.c fixes the issue for me. But I'm > > > not sure if there is a more generic fix to that problem. > > > > Thanks for the update. What do you mean that it changes the numbering, > > do you get a compile time error or are you saying that it is the order > > of parsing the options that change? > > > > Are you building on Linux or another OS? > > We are doing KVM testing, so it is Linux. > > What I did is putting lines like this somewhere into vl.c and > os-posix.c: > fprintf(stderr, "os: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); > fprintf(stderr, "vl: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); > > Resulting in the following output on stderr: > os: QEMU_OPTION_daemonize: 85 > vl: QEMU_OPTION_daemonize: 86 > > No compile time errors. The preprocessing of qemu-options.h is done > separately for both files. This results in a missing option definition > for os-posix.c and discrepancy in the option enumeration. Sorry, missed the part where your patch comes into play: From vl.c the function os_parse_cmd_args is called with option index 86, and the switch statement in os-posix.c's os_parse_cmd_args checks for 85 to set the daemonize. Obviously, this wont work. -- Frank
On 06/25/10 19:34, Frank Arnold wrote: > We are doing KVM testing, so it is Linux. > > What I did is putting lines like this somewhere into vl.c and > os-posix.c: > fprintf(stderr, "os: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); > fprintf(stderr, "vl: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); > > Resulting in the following output on stderr: > os: QEMU_OPTION_daemonize: 85 > vl: QEMU_OPTION_daemonize: 86 > > No compile time errors. The preprocessing of qemu-options.h is done > separately for both files. This results in a missing option definition > for os-posix.c and discrepancy in the option enumeration. Ok this is truly odd! Let me try and see if I can reproduce it here. Cheers, Jes
On 06/25/10 19:34, Frank Arnold wrote: > We are doing KVM testing, so it is Linux. > > What I did is putting lines like this somewhere into vl.c and > os-posix.c: > fprintf(stderr, "os: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); > fprintf(stderr, "vl: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); > > Resulting in the following output on stderr: > os: QEMU_OPTION_daemonize: 85 > vl: QEMU_OPTION_daemonize: 86 > > No compile time errors. The preprocessing of qemu-options.h is done > separately for both files. This results in a missing option definition > for os-posix.c and discrepancy in the option enumeration. Hi Frank, I figured out what was causing it. qemu-options.def has an #ifdef MAP_POPULATE in it, which isn't being set without sys/mmap.h being included. Pretty much every other #ifdef in qemu-options.def are based on CONFIG_foo settings or things like _WIN32 which do not change depending on header file inclusion. I think the easiest fix is to just add sys/mmap.h to the include list in os-posix.c, so I just posted a patch for that. Though, in principle we really shouldn't base qemu-options.def settings on defines pulled in from system header files. Cheers, Jes
On Mon, Jun 28, 2010 at 2:50 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: > On 06/25/10 19:34, Frank Arnold wrote: >> We are doing KVM testing, so it is Linux. >> >> What I did is putting lines like this somewhere into vl.c and >> os-posix.c: >> fprintf(stderr, "os: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); >> fprintf(stderr, "vl: QEMU_OPTION_daemonize: %i", QEMU_OPTION_daemonize); >> >> Resulting in the following output on stderr: >> os: QEMU_OPTION_daemonize: 85 >> vl: QEMU_OPTION_daemonize: 86 >> >> No compile time errors. The preprocessing of qemu-options.h is done >> separately for both files. This results in a missing option definition >> for os-posix.c and discrepancy in the option enumeration. > > Hi Frank, > > I figured out what was causing it. qemu-options.def has an > #ifdef MAP_POPULATE in it, which isn't being set without sys/mmap.h > being included. Pretty much every other #ifdef in qemu-options.def are > based on CONFIG_foo settings or things like _WIN32 which do not change > depending on header file inclusion. > > I think the easiest fix is to just add sys/mmap.h to the include list in > os-posix.c, so I just posted a patch for that. Though, in principle we > really shouldn't base qemu-options.def settings on defines pulled in > from system header files. I think more flags should be added to arch_mask field, like QEMU_ARCH_LINUX, QEMU_ARCH_POSIX and QEMU_ARCH_WIN32. Then the #ifdefs should be removed. Prealloc command line flag stuff should be conditional to CONFIG_LINUX only, there should be another check for MAP_POPULATE where mem_preallocate is set. Alternatively, we could have more arch_mask flags like QEMU_MAP_POPULATE.
On 06/28/10 17:42, Blue Swirl wrote: > On Mon, Jun 28, 2010 at 2:50 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: >> I figured out what was causing it. qemu-options.def has an >> #ifdef MAP_POPULATE in it, which isn't being set without sys/mmap.h >> being included. Pretty much every other #ifdef in qemu-options.def are >> based on CONFIG_foo settings or things like _WIN32 which do not change >> depending on header file inclusion. >> >> I think the easiest fix is to just add sys/mmap.h to the include list in >> os-posix.c, so I just posted a patch for that. Though, in principle we >> really shouldn't base qemu-options.def settings on defines pulled in >> from system header files. > > I think more flags should be added to arch_mask field, like > QEMU_ARCH_LINUX, QEMU_ARCH_POSIX and QEMU_ARCH_WIN32. Then the #ifdefs > should be removed. Prealloc command line flag stuff should be > conditional to CONFIG_LINUX only, there should be another check for > MAP_POPULATE where mem_preallocate is set. > > Alternatively, we could have more arch_mask flags like QEMU_MAP_POPULATE. Yeah, the problem with tying it to CONFIG_LINUX is that older version of Linux may not support it. Looking through the list, MAP_POPULATE is really an oddball in there though, so maybe it would be cleaner to catch it via configure and then use CONFIG_MAP_POPULATE or something like that? Cheers, Jes
On Mon, Jun 28, 2010 at 4:03 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: > On 06/28/10 17:42, Blue Swirl wrote: >> On Mon, Jun 28, 2010 at 2:50 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: >>> I figured out what was causing it. qemu-options.def has an >>> #ifdef MAP_POPULATE in it, which isn't being set without sys/mmap.h >>> being included. Pretty much every other #ifdef in qemu-options.def are >>> based on CONFIG_foo settings or things like _WIN32 which do not change >>> depending on header file inclusion. >>> >>> I think the easiest fix is to just add sys/mmap.h to the include list in >>> os-posix.c, so I just posted a patch for that. Though, in principle we >>> really shouldn't base qemu-options.def settings on defines pulled in >>> from system header files. >> >> I think more flags should be added to arch_mask field, like >> QEMU_ARCH_LINUX, QEMU_ARCH_POSIX and QEMU_ARCH_WIN32. Then the #ifdefs >> should be removed. Prealloc command line flag stuff should be >> conditional to CONFIG_LINUX only, there should be another check for >> MAP_POPULATE where mem_preallocate is set. >> >> Alternatively, we could have more arch_mask flags like QEMU_MAP_POPULATE. > > Yeah, the problem with tying it to CONFIG_LINUX is that older version of > Linux may not support it. Looking through the list, MAP_POPULATE is > really an oddball in there though, so maybe it would be cleaner to catch > it via configure and then use CONFIG_MAP_POPULATE or something like that? There'd be 1:1 relation between MAP_POPULATE and CONFIG_MAP_POPULATE, so maybe not.
On 06/28/10 18:20, Blue Swirl wrote: > On Mon, Jun 28, 2010 at 4:03 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote: >> Yeah, the problem with tying it to CONFIG_LINUX is that older version of >> Linux may not support it. Looking through the list, MAP_POPULATE is >> really an oddball in there though, so maybe it would be cleaner to catch >> it via configure and then use CONFIG_MAP_POPULATE or something like that? > > There'd be 1:1 relation between MAP_POPULATE and CONFIG_MAP_POPULATE, > so maybe not. That is correct, but CONFIG_MAP_POPULATE would be in config.h so it would not get missed out in cases where someone includes qemu-options.h without including sys/mmap.h first. But it is a corner case, so my patch should be fine. Cheers, Jes
On 06/28/2010 06:03 PM, Jes Sorensen wrote: > On 06/28/10 17:42, Blue Swirl wrote: >> On Mon, Jun 28, 2010 at 2:50 PM, Jes Sorensen<Jes.Sorensen@redhat.com> wrote: >>> I figured out what was causing it. qemu-options.def has an >>> #ifdef MAP_POPULATE in it, which isn't being set without sys/mmap.h >>> being included. Pretty much every other #ifdef in qemu-options.def are >>> based on CONFIG_foo settings or things like _WIN32 which do not change >>> depending on header file inclusion. >>> >>> I think the easiest fix is to just add sys/mmap.h to the include list in >>> os-posix.c, so I just posted a patch for that. Though, in principle we >>> really shouldn't base qemu-options.def settings on defines pulled in >>> from system header files. >> >> I think more flags should be added to arch_mask field, like >> QEMU_ARCH_LINUX, QEMU_ARCH_POSIX and QEMU_ARCH_WIN32. Then the #ifdefs >> should be removed. Prealloc command line flag stuff should be >> conditional to CONFIG_LINUX only, there should be another check for >> MAP_POPULATE where mem_preallocate is set. >> >> Alternatively, we could have more arch_mask flags like QEMU_MAP_POPULATE. > > Yeah, the problem with tying it to CONFIG_LINUX is that older version of > Linux may not support it. Looking through the list, MAP_POPULATE is > really an oddball in there though, so maybe it would be cleaner to catch > it via configure and then use CONFIG_MAP_POPULATE or something like that? Or create a header file system.h that pulls all we need from the system, and remove (almost) all <...> includes from elsewhere. Paolo
diff --git a/os-posix.c b/os-posix.c index 6417d16..1672e06 100644 --- a/os-posix.c +++ b/os-posix.c @@ -39,6 +39,8 @@ static struct passwd *user_pwd; static const char *chroot_dir; +static int daemonize; +static int fds[2]; void os_setup_early_signal_handling(void) { @@ -160,6 +162,9 @@ void os_parse_cmd_args(int index, const char *optarg) case QEMU_OPTION_chroot: chroot_dir = optarg; break; + case QEMU_OPTION_daemonize: + daemonize = 1; + break; } return; } @@ -196,3 +201,100 @@ void os_change_root(void) } } + +void os_daemonize(void) +{ + if (daemonize) { + pid_t pid; + + if (pipe(fds) == -1) + exit(1); + + pid = fork(); + if (pid > 0) { + uint8_t status; + ssize_t len; + + close(fds[1]); + + again: + len = read(fds[0], &status, 1); + if (len == -1 && (errno == EINTR)) + goto again; + + if (len != 1) + exit(1); + else if (status == 1) { + fprintf(stderr, "Could not acquire pidfile: %s\n", strerror(errno)); + exit(1); + } else + exit(0); + } else if (pid < 0) + exit(1); + + close(fds[0]); + qemu_set_cloexec(fds[1]); + + setsid(); + + pid = fork(); + if (pid > 0) + exit(0); + else if (pid < 0) + exit(1); + + umask(027); + + signal(SIGTSTP, SIG_IGN); + signal(SIGTTOU, SIG_IGN); + signal(SIGTTIN, SIG_IGN); + } +} + +void os_setup_post(void) +{ + int fd = 0; + + if (daemonize) { + uint8_t status = 0; + ssize_t len; + + again1: + len = write(fds[1], &status, 1); + if (len == -1 && (errno == EINTR)) + goto again1; + + if (len != 1) + exit(1); + + if (chdir("/")) { + perror("not able to chdir to /"); + exit(1); + } + TFR(fd = qemu_open("/dev/null", O_RDWR)); + if (fd == -1) + exit(1); + } + + os_change_root(); + os_change_process_uid(); + + if (daemonize) { + dup2(fd, 0); + dup2(fd, 1); + dup2(fd, 2); + + close(fd); + } +} + +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)); +} diff --git a/os-win32.c b/os-win32.c index aefc535..d98fd77 100644 --- a/os-win32.c +++ b/os-win32.c @@ -214,3 +214,8 @@ void os_parse_cmd_args(int index, const char *optarg) { return; } + +void os_pidfile_error(void) +{ + fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno)); +} diff --git a/qemu-os-posix.h b/qemu-os-posix.h index 91c7b68..9b07660 100644 --- a/qemu-os-posix.h +++ b/qemu-os-posix.h @@ -33,5 +33,7 @@ static inline void os_host_main_loop_wait(int *timeout) void os_setup_signal_handling(void); void os_change_process_uid(void); void os_change_root(void); +void os_daemonize(void); +void os_setup_post(void); #endif diff --git a/qemu-os-win32.h b/qemu-os-win32.h index e2a97d2..c4aa84a 100644 --- a/qemu-os-win32.h +++ b/qemu-os-win32.h @@ -45,5 +45,7 @@ void os_host_main_loop_wait(int *timeout); static inline void os_setup_signal_handling(void) {} static inline void os_change_process_uid(void) {} static inline void os_change_root(void) {} +static inline void os_daemonize(void) {} +static inline void os_setup_post(void) {} #endif diff --git a/sysemu.h b/sysemu.h index 2162b1d..346cccd 100644 --- a/sysemu.h +++ b/sysemu.h @@ -83,6 +83,7 @@ void do_info_slirp(Monitor *mon); void os_setup_early_signal_handling(void); char *os_find_datadir(const char *argv0); void os_parse_cmd_args(int index, const char *optarg); +void os_pidfile_error(void); typedef enum DisplayType { diff --git a/vl.c b/vl.c index c5f0e63..8902477 100644 --- a/vl.c +++ b/vl.c @@ -216,9 +216,6 @@ int no_shutdown = 0; int cursor_hide = 1; int graphic_rotate = 0; uint8_t irq0override = 1; -#ifndef _WIN32 -int daemonize = 0; -#endif const char *watchdog; const char *option_rom[MAX_OPTION_ROMS]; int nb_option_roms; @@ -2301,15 +2298,9 @@ int main(int argc, char **argv, char **envp) const char *loadvm = NULL; QEMUMachine *machine; const char *cpu_model; -#ifndef _WIN32 - int fds[2]; -#endif int tb_size; const char *pid_file = NULL; const char *incoming = NULL; -#ifndef _WIN32 - int fd = 0; -#endif int show_vnc_port = 0; int defconfig = 1; @@ -2975,11 +2966,6 @@ int main(int argc, char **argv, char **envp) exit(1); } break; -#ifndef _WIN32 - case QEMU_OPTION_daemonize: - daemonize = 1; - break; -#endif case QEMU_OPTION_option_rom: if (nb_option_roms >= MAX_OPTION_ROMS) { fprintf(stderr, "Too many option ROMs\n"); @@ -3194,64 +3180,10 @@ int main(int argc, char **argv, char **envp) } #endif -#ifndef _WIN32 - if (daemonize) { - pid_t pid; - - if (pipe(fds) == -1) - exit(1); - - pid = fork(); - if (pid > 0) { - uint8_t status; - ssize_t len; - - close(fds[1]); - - again: - len = read(fds[0], &status, 1); - if (len == -1 && (errno == EINTR)) - goto again; - - if (len != 1) - exit(1); - else if (status == 1) { - fprintf(stderr, "Could not acquire pidfile: %s\n", strerror(errno)); - exit(1); - } else - exit(0); - } else if (pid < 0) - exit(1); - - close(fds[0]); - qemu_set_cloexec(fds[1]); - - setsid(); - - pid = fork(); - if (pid > 0) - exit(0); - else if (pid < 0) - exit(1); - - umask(027); - - signal(SIGTSTP, SIG_IGN); - signal(SIGTTOU, SIG_IGN); - signal(SIGTTIN, SIG_IGN); - } -#endif + os_daemonize(); if (pid_file && qemu_create_pidfile(pid_file) != 0) { -#ifndef _WIN32 - if (daemonize) { - uint8_t status = 1; - if (write(fds[1], &status, 1) != 1) { - perror("daemonize. Writing to pipe\n"); - } - } else -#endif - fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno)); + os_pidfile_error(); exit(1); } @@ -3519,39 +3451,7 @@ int main(int argc, char **argv, char **envp) vm_start(); } -#ifndef _WIN32 - if (daemonize) { - uint8_t status = 0; - ssize_t len; - - again1: - len = write(fds[1], &status, 1); - if (len == -1 && (errno == EINTR)) - goto again1; - - if (len != 1) - exit(1); - - if (chdir("/")) { - perror("not able to chdir to /"); - exit(1); - } - TFR(fd = qemu_open("/dev/null", O_RDWR)); - if (fd == -1) - exit(1); - } - - os_change_root(); - os_change_process_uid(); - - if (daemonize) { - dup2(fd, 0); - dup2(fd, 1); - dup2(fd, 2); - - close(fd); - } -#endif + os_setup_post(); main_loop(); quit_timers();