Patchwork [v2,15/39] vl: init main loop earlier

login
register
mail settings
Submitter Paolo Bonzini
Date Nov. 2, 2012, 9:55 a.m.
Message ID <5093987B.7030201@redhat.com>
Download mbox | patch
Permalink /patch/196521/
State New
Headers show

Comments

Paolo Bonzini - Nov. 2, 2012, 9:55 a.m.
Il 02/11/2012 08:26, Jan Kiszka ha scritto:
> Can we move os_daemonize before that, or what are its dependencies? I
> have an increasingly bad feeling about this code shuffling.

Moving os_daemonize too early ruins error messages.  I would just
pull qemu_mutex_lock_iothread later, even just before cpu_exec_init_all.
Something like this (untested):
Jan Kiszka - Nov. 2, 2012, 10:32 a.m.
On 2012-11-02 10:55, Paolo Bonzini wrote:
> Il 02/11/2012 08:26, Jan Kiszka ha scritto:
>> Can we move os_daemonize before that, or what are its dependencies? I
>> have an increasingly bad feeling about this code shuffling.
> 
> Moving os_daemonize too early ruins error messages.  I would just
> pull qemu_mutex_lock_iothread later, even just before cpu_exec_init_all.
> Something like this (untested):
> 
> diff --git a/main-loop.c b/main-loop.c
> index e43c7c8..07c4b84 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -125,7 +125,6 @@ int qemu_init_main_loop(void)
>      init_clocks();
>      init_timer_alarm();
>  
> -    qemu_mutex_lock_iothread();
>      ret = qemu_signal_init();
>      if (ret) {
>          return ret;
> diff --git a/vl.c b/vl.c
> index 99681da..210d525 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3631,13 +3631,6 @@ int main(int argc, char **argv, char **envp)
>      }
>  #endif
>  
> -    os_daemonize();
> -
> -    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
> -        os_pidfile_error();
> -        exit(1);
> -    }
> -
>      /* init the memory */
>      if (ram_size == 0) {
>          ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
> @@ -3682,11 +3675,6 @@ int main(int argc, char **argv, char **envp)
>  
>      os_set_line_buffering();
>  
> -#ifdef CONFIG_SPICE
> -    /* spice needs the timers to be initialized by this point */
> -    qemu_spice_init();
> -#endif
> -
>      if (icount_option && (kvm_enabled() || xen_enabled())) {
>          fprintf(stderr, "-icount is not allowed with kvm or xen\n");
>          exit(1);
> @@ -3709,6 +3697,20 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> +    os_daemonize();
> +
> +    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
> +        os_pidfile_error();
> +        exit(1);
> +    }
> +
> +    qemu_mutex_lock_iothread();
> +
> +#ifdef CONFIG_SPICE
> +    /* spice needs timers & threads to be initialized by this point */
> +    qemu_spice_init();
> +#endif
> +
>      cpu_exec_init_all();
>  
>      bdrv_init_with_whitelist();
> 

Would have been too easy:

# qemu-system-x86_64 -daemonize
gettime: Invalid argument
Internal timer error: aborting

Haven't looked at details yet.

Jan
Paolo Bonzini - Nov. 2, 2012, 11:04 a.m.
Il 02/11/2012 11:32, Jan Kiszka ha scritto:
> On 2012-11-02 10:55, Paolo Bonzini wrote:
>> Il 02/11/2012 08:26, Jan Kiszka ha scritto:
>>> Can we move os_daemonize before that, or what are its dependencies? I
>>> have an increasingly bad feeling about this code shuffling.
>>
>> Moving os_daemonize too early ruins error messages.  I would just
>> pull qemu_mutex_lock_iothread later, even just before cpu_exec_init_all.
>> Something like this (untested):
>>
>> diff --git a/main-loop.c b/main-loop.c
>> index e43c7c8..07c4b84 100644
>> --- a/main-loop.c
>> +++ b/main-loop.c
>> @@ -125,7 +125,6 @@ int qemu_init_main_loop(void)
>>      init_clocks();
>>      init_timer_alarm();
>>  
>> -    qemu_mutex_lock_iothread();
>>      ret = qemu_signal_init();
>>      if (ret) {
>>          return ret;
>> diff --git a/vl.c b/vl.c
>> index 99681da..210d525 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3631,13 +3631,6 @@ int main(int argc, char **argv, char **envp)
>>      }
>>  #endif
>>  
>> -    os_daemonize();
>> -
>> -    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
>> -        os_pidfile_error();
>> -        exit(1);
>> -    }
>> -
>>      /* init the memory */
>>      if (ram_size == 0) {
>>          ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
>> @@ -3682,11 +3675,6 @@ int main(int argc, char **argv, char **envp)
>>  
>>      os_set_line_buffering();
>>  
>> -#ifdef CONFIG_SPICE
>> -    /* spice needs the timers to be initialized by this point */
>> -    qemu_spice_init();
>> -#endif
>> -
>>      if (icount_option && (kvm_enabled() || xen_enabled())) {
>>          fprintf(stderr, "-icount is not allowed with kvm or xen\n");
>>          exit(1);
>> @@ -3709,6 +3697,20 @@ int main(int argc, char **argv, char **envp)
>>          }
>>      }
>>  
>> +    os_daemonize();
>> +
>> +    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
>> +        os_pidfile_error();
>> +        exit(1);
>> +    }
>> +
>> +    qemu_mutex_lock_iothread();
>> +
>> +#ifdef CONFIG_SPICE
>> +    /* spice needs timers & threads to be initialized by this point */
>> +    qemu_spice_init();
>> +#endif
>> +
>>      cpu_exec_init_all();
>>  
>>      bdrv_init_with_whitelist();
>>
> 
> Would have been too easy:
> 
> # qemu-system-x86_64 -daemonize
> gettime: Invalid argument
> Internal timer error: aborting
> 
> Haven't looked at details yet.

np, will do so now.

Paolo

Patch

diff --git a/main-loop.c b/main-loop.c
index e43c7c8..07c4b84 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -125,7 +125,6 @@  int qemu_init_main_loop(void)
     init_clocks();
     init_timer_alarm();
 
-    qemu_mutex_lock_iothread();
     ret = qemu_signal_init();
     if (ret) {
         return ret;
diff --git a/vl.c b/vl.c
index 99681da..210d525 100644
--- a/vl.c
+++ b/vl.c
@@ -3631,13 +3631,6 @@  int main(int argc, char **argv, char **envp)
     }
 #endif
 
-    os_daemonize();
-
-    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
-        os_pidfile_error();
-        exit(1);
-    }
-
     /* init the memory */
     if (ram_size == 0) {
         ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
@@ -3682,11 +3675,6 @@  int main(int argc, char **argv, char **envp)
 
     os_set_line_buffering();
 
-#ifdef CONFIG_SPICE
-    /* spice needs the timers to be initialized by this point */
-    qemu_spice_init();
-#endif
-
     if (icount_option && (kvm_enabled() || xen_enabled())) {
         fprintf(stderr, "-icount is not allowed with kvm or xen\n");
         exit(1);
@@ -3709,6 +3697,20 @@  int main(int argc, char **argv, char **envp)
         }
     }
 
+    os_daemonize();
+
+    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
+        os_pidfile_error();
+        exit(1);
+    }
+
+    qemu_mutex_lock_iothread();
+
+#ifdef CONFIG_SPICE
+    /* spice needs timers & threads to be initialized by this point */
+    qemu_spice_init();
+#endif
+
     cpu_exec_init_all();
 
     bdrv_init_with_whitelist();