| Message ID | 1596122076-341293-13-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Live Update | expand |
On 7/30/20 10:14 AM, Steve Sistare wrote: > Provide the -pause command-line parameter and the QEMU_PAUSE environment > variable to briefly pause QEMU in main and allow a developer to attach gdb. > Useful when the developer does not invoke QEMU directly, such as when using > libvirt. How would you set this option with libvirt? It feels like you are trying to reinvent something that is already well-documented: https://www.berrange.com/posts/2011/10/12/debugging-early-startup-of-kvm-with-gdb-when-launched-by-libvirtd/ > > Usage: > qemu -pause <seconds> > or > export QEMU_PAUSE=<seconds> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > qemu-options.hx | 9 +++++++++ > softmmu/vl.c | 15 ++++++++++++++- > 2 files changed, 23 insertions(+), 1 deletion(-) > @@ -3204,6 +3211,12 @@ void qemu_init(int argc, char **argv, char **envp) > case QEMU_OPTION_gdb: > add_device_config(DEV_GDB, optarg); > break; > + case QEMU_OPTION_pause: > + seconds = atoi(optarg); atoi() cannot detect overflow. You should never use it in robust parsing of untrusted input.
Steve Sistare <steven.sistare@oracle.com> writes: > Provide the -pause command-line parameter and the QEMU_PAUSE environment > variable to briefly pause QEMU in main and allow a developer to attach gdb. > Useful when the developer does not invoke QEMU directly, such as when using > libvirt. How does this differ from -S? > > Usage: > qemu -pause <seconds> > or > export QEMU_PAUSE=<seconds> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > qemu-options.hx | 9 +++++++++ > softmmu/vl.c | 15 ++++++++++++++- > 2 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/qemu-options.hx b/qemu-options.hx > index 708583b..8505cf2 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3668,6 +3668,15 @@ SRST > option is experimental. > ERST > > +DEF("pause", HAS_ARG, QEMU_OPTION_pause, \ > + "-pause secs Pause for secs seconds on entry to main.\n", QEMU_ARCH_ALL) > + > +SRST > +``--pause secs`` > + Pause for a number of seconds on entry to main. Useful for attaching > + a debugger after QEMU has been launched by some other entity. > +ERST > + It seems like having an option to race with the debugger is just asking for trouble. > DEF("S", 0, QEMU_OPTION_S, \ > "-S freeze CPU at startup (use 'c' to start execution)\n", > QEMU_ARCH_ALL) > diff --git a/softmmu/vl.c b/softmmu/vl.c > index 8478778..951994f 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -2844,7 +2844,7 @@ static void create_default_memdev(MachineState *ms, const char *path) > > void qemu_init(int argc, char **argv, char **envp) > { > - int i; > + int i, seconds; > int snapshot, linux_boot; > const char *initrd_filename; > const char *kernel_filename, *kernel_cmdline; > @@ -2882,6 +2882,13 @@ void qemu_init(int argc, char **argv, char **envp) > QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list); > int mem_prealloc = 0; /* force preallocation of physical target memory */ > > + if (getenv("QEMU_PAUSE")) { > + seconds = atoi(getenv("QEMU_PAUSE")); > + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", > + seconds, getpid()); > + sleep(seconds); > + } > + > os_set_line_buffering(); > > error_init(argv[0]); > @@ -3204,6 +3211,12 @@ void qemu_init(int argc, char **argv, char **envp) > case QEMU_OPTION_gdb: > add_device_config(DEV_GDB, optarg); > break; > + case QEMU_OPTION_pause: > + seconds = atoi(optarg); > + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", > + seconds, getpid()); > + sleep(seconds); > + break; > case QEMU_OPTION_L: > if (is_help_option(optarg)) { > list_data_dirs = true;
On 7/30/2020 12:20 PM, Eric Blake wrote: > On 7/30/20 10:14 AM, Steve Sistare wrote: >> Provide the -pause command-line parameter and the QEMU_PAUSE environment >> variable to briefly pause QEMU in main and allow a developer to attach gdb. >> Useful when the developer does not invoke QEMU directly, such as when using >> libvirt. > > How would you set this option with libvirt? Add -pause in the qemu args in the xml. > It feels like you are trying to reinvent something that is already well-documented: > > https://www.berrange.com/posts/2011/10/12/debugging-early-startup-of-kvm-with-gdb-when-launched-by-libvirtd/ Too many steps to reach BINGO for my taste. Easier is better. Also, in our shop we start qemu in other ways, such as via services. These new hooks helped me and my colleagues, and I hope others may also find them useful, but if not then we drop them. >> Usage: >> qemu -pause <seconds> >> or >> export QEMU_PAUSE=<seconds> >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> qemu-options.hx | 9 +++++++++ >> softmmu/vl.c | 15 ++++++++++++++- >> 2 files changed, 23 insertions(+), 1 deletion(-) > >> @@ -3204,6 +3211,12 @@ void qemu_init(int argc, char **argv, char **envp) >> case QEMU_OPTION_gdb: >> add_device_config(DEV_GDB, optarg); >> break; >> + case QEMU_OPTION_pause: >> + seconds = atoi(optarg); > > atoi() cannot detect overflow. You should never use it in robust parsing of untrusted input. OK. - Steve
On 7/30/2020 1:03 PM, Alex Bennée wrote: > > Steve Sistare <steven.sistare@oracle.com> writes: > >> Provide the -pause command-line parameter and the QEMU_PAUSE environment >> variable to briefly pause QEMU in main and allow a developer to attach gdb. >> Useful when the developer does not invoke QEMU directly, such as when using >> libvirt. > > How does this differ from -S? The -S flag runs qemu to the main loop but does not start the guest. Lots of code that you may need to debug runs before you get there. - Steve >> Usage: >> qemu -pause <seconds> >> or >> export QEMU_PAUSE=<seconds> >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> qemu-options.hx | 9 +++++++++ >> softmmu/vl.c | 15 ++++++++++++++- >> 2 files changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 708583b..8505cf2 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -3668,6 +3668,15 @@ SRST >> option is experimental. >> ERST >> >> +DEF("pause", HAS_ARG, QEMU_OPTION_pause, \ >> + "-pause secs Pause for secs seconds on entry to main.\n", QEMU_ARCH_ALL) >> + >> +SRST >> +``--pause secs`` >> + Pause for a number of seconds on entry to main. Useful for attaching >> + a debugger after QEMU has been launched by some other entity. >> +ERST >> + > > It seems like having an option to race with the debugger is just asking > for trouble. > >> DEF("S", 0, QEMU_OPTION_S, \ >> "-S freeze CPU at startup (use 'c' to start execution)\n", >> QEMU_ARCH_ALL) >> diff --git a/softmmu/vl.c b/softmmu/vl.c >> index 8478778..951994f 100644 >> --- a/softmmu/vl.c >> +++ b/softmmu/vl.c >> @@ -2844,7 +2844,7 @@ static void create_default_memdev(MachineState *ms, const char *path) >> >> void qemu_init(int argc, char **argv, char **envp) >> { >> - int i; >> + int i, seconds; >> int snapshot, linux_boot; >> const char *initrd_filename; >> const char *kernel_filename, *kernel_cmdline; >> @@ -2882,6 +2882,13 @@ void qemu_init(int argc, char **argv, char **envp) >> QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list); >> int mem_prealloc = 0; /* force preallocation of physical target memory */ >> >> + if (getenv("QEMU_PAUSE")) { >> + seconds = atoi(getenv("QEMU_PAUSE")); >> + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", >> + seconds, getpid()); >> + sleep(seconds); >> + } >> + >> os_set_line_buffering(); >> >> error_init(argv[0]); >> @@ -3204,6 +3211,12 @@ void qemu_init(int argc, char **argv, char **envp) >> case QEMU_OPTION_gdb: >> add_device_config(DEV_GDB, optarg); >> break; >> + case QEMU_OPTION_pause: >> + seconds = atoi(optarg); >> + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", >> + seconds, getpid()); >> + sleep(seconds); >> + break; >> case QEMU_OPTION_L: >> if (is_help_option(optarg)) { >> list_data_dirs = true; > >
Steven Sistare <steven.sistare@oracle.com> writes: > On 7/30/2020 1:03 PM, Alex Bennée wrote: >> >> Steve Sistare <steven.sistare@oracle.com> writes: >> >>> Provide the -pause command-line parameter and the QEMU_PAUSE environment >>> variable to briefly pause QEMU in main and allow a developer to attach gdb. >>> Useful when the developer does not invoke QEMU directly, such as when using >>> libvirt. >> >> How does this differ from -S? > > The -S flag runs qemu to the main loop but does not start the guest. Lots of code > that you may need to debug runs before you get there. Right - so this is for attaching a debugger to QEMU itself, not using the gdbstub? Why isn't this a problem the calling entity can solve by the way it invoked QEMU? We have similar sort of solutions for debugging our testcases: https://wiki.qemu.org/Features/QTest#Using_debugging_tools_under_the_test_harness I still think: >>> +DEF("pause", HAS_ARG, QEMU_OPTION_pause, \ >>> + "-pause secs Pause for secs seconds on entry to main.\n", QEMU_ARCH_ALL) >>> + >>> +SRST >>> +``--pause secs`` >>> + Pause for a number of seconds on entry to main. Useful for attaching >>> + a debugger after QEMU has been launched by some other entity. >>> +ERST >>> + >> >> It seems like having an option to race with the debugger is just asking >> for trouble. this make the option problematic.
On Thu, Jul 30, 2020 at 02:11:19PM -0400, Steven Sistare wrote: > On 7/30/2020 12:20 PM, Eric Blake wrote: > > On 7/30/20 10:14 AM, Steve Sistare wrote: > >> Provide the -pause command-line parameter and the QEMU_PAUSE environment > >> variable to briefly pause QEMU in main and allow a developer to attach gdb. > >> Useful when the developer does not invoke QEMU directly, such as when using > >> libvirt. > > > > How would you set this option with libvirt? > > Add -pause in the qemu args in the xml. > > > It feels like you are trying to reinvent something that is already well-documented: > > > > https://www.berrange.com/posts/2011/10/12/debugging-early-startup-of-kvm-with-gdb-when-launched-by-libvirtd/ > > Too many steps to reach BINGO for my taste. Easier is better. Also, in our shop we start qemu > in other ways, such as via services. A "sleep" is a pretty crude & unreliable way to get into debugging though. It is racy for a start, but also QEMU has a bunch of stuff that runs via ELF constructors before main() even starts. So I feel like the thing that starts QEMU is better placed to provide a way in for debugging. eg the service launcher can send SIGSTOP to the child process immediately before the execve(qemu) call. Now user can attach with the debugger, allow execution to continue, and has ability to debug *everything* right from the ELF constructors onwards into main() and all that follows. Regards, Daniel
On 7/31/2020 6:07 AM, Daniel P. Berrangé wrote: > On Thu, Jul 30, 2020 at 02:11:19PM -0400, Steven Sistare wrote: >> On 7/30/2020 12:20 PM, Eric Blake wrote: >>> On 7/30/20 10:14 AM, Steve Sistare wrote: >>>> Provide the -pause command-line parameter and the QEMU_PAUSE environment >>>> variable to briefly pause QEMU in main and allow a developer to attach gdb. >>>> Useful when the developer does not invoke QEMU directly, such as when using >>>> libvirt. >>> >>> How would you set this option with libvirt? >> >> Add -pause in the qemu args in the xml. >> >>> It feels like you are trying to reinvent something that is already well-documented: >>> >>> https://www.berrange.com/posts/2011/10/12/debugging-early-startup-of-kvm-with-gdb-when-launched-by-libvirtd/ >> >> Too many steps to reach BINGO for my taste. Easier is better. Also, in our shop we start qemu >> in other ways, such as via services. > > A "sleep" is a pretty crude & unreliable way to get into debugging > though. It is racy for a start, but also QEMU has a bunch of stuff > that runs via ELF constructors before main() even starts. > > So I feel like the thing that starts QEMU is better placed to provide > a way in for debugging. > > eg the service launcher can send SIGSTOP to the child process immediately > before the execve(qemu) call. > > Now user can attach with the debugger, allow execution to continue, > and has ability to debug *everything* right from the ELF constructors > onwards into main() and all that follows. > > Regards, > Daniel That is a nice solution for the launchers we can modify. We could use your idea in place of the sleep in main, kill(getpid(), SIGSTOP); Not quite as good as being able to debug the elf constructors, but still helpful. - Steve
* Steven Sistare (steven.sistare@oracle.com) wrote: > On 7/30/2020 1:03 PM, Alex Bennée wrote: > > > > Steve Sistare <steven.sistare@oracle.com> writes: > > > >> Provide the -pause command-line parameter and the QEMU_PAUSE environment > >> variable to briefly pause QEMU in main and allow a developer to attach gdb. > >> Useful when the developer does not invoke QEMU directly, such as when using > >> libvirt. > > > > How does this differ from -S? > > The -S flag runs qemu to the main loop but does not start the guest. Lots of code > that you may need to debug runs before you get there. You might try the '--preconfig' option - that's pretty early on. The other one is adding a chardev and telling it to wait for a server; that'll wait until you telnet to the port. (Either way, this patch shouldn't really be part of this series, it's a separate discussion) Dave > - Steve > >> Usage: > >> qemu -pause <seconds> > >> or > >> export QEMU_PAUSE=<seconds> > >> > >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > >> --- > >> qemu-options.hx | 9 +++++++++ > >> softmmu/vl.c | 15 ++++++++++++++- > >> 2 files changed, 23 insertions(+), 1 deletion(-) > >> > >> diff --git a/qemu-options.hx b/qemu-options.hx > >> index 708583b..8505cf2 100644 > >> --- a/qemu-options.hx > >> +++ b/qemu-options.hx > >> @@ -3668,6 +3668,15 @@ SRST > >> option is experimental. > >> ERST > >> > >> +DEF("pause", HAS_ARG, QEMU_OPTION_pause, \ > >> + "-pause secs Pause for secs seconds on entry to main.\n", QEMU_ARCH_ALL) > >> + > >> +SRST > >> +``--pause secs`` > >> + Pause for a number of seconds on entry to main. Useful for attaching > >> + a debugger after QEMU has been launched by some other entity. > >> +ERST > >> + > > > > It seems like having an option to race with the debugger is just asking > > for trouble. > > > >> DEF("S", 0, QEMU_OPTION_S, \ > >> "-S freeze CPU at startup (use 'c' to start execution)\n", > >> QEMU_ARCH_ALL) > >> diff --git a/softmmu/vl.c b/softmmu/vl.c > >> index 8478778..951994f 100644 > >> --- a/softmmu/vl.c > >> +++ b/softmmu/vl.c > >> @@ -2844,7 +2844,7 @@ static void create_default_memdev(MachineState *ms, const char *path) > >> > >> void qemu_init(int argc, char **argv, char **envp) > >> { > >> - int i; > >> + int i, seconds; > >> int snapshot, linux_boot; > >> const char *initrd_filename; > >> const char *kernel_filename, *kernel_cmdline; > >> @@ -2882,6 +2882,13 @@ void qemu_init(int argc, char **argv, char **envp) > >> QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list); > >> int mem_prealloc = 0; /* force preallocation of physical target memory */ > >> > >> + if (getenv("QEMU_PAUSE")) { > >> + seconds = atoi(getenv("QEMU_PAUSE")); > >> + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", > >> + seconds, getpid()); > >> + sleep(seconds); > >> + } > >> + > >> os_set_line_buffering(); > >> > >> error_init(argv[0]); > >> @@ -3204,6 +3211,12 @@ void qemu_init(int argc, char **argv, char **envp) > >> case QEMU_OPTION_gdb: > >> add_device_config(DEV_GDB, optarg); > >> break; > >> + case QEMU_OPTION_pause: > >> + seconds = atoi(optarg); > >> + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", > >> + seconds, getpid()); > >> + sleep(seconds); > >> + break; > >> case QEMU_OPTION_L: > >> if (is_help_option(optarg)) { > >> list_data_dirs = true; > > > > >
On 9/11/2020 1:59 PM, Dr. David Alan Gilbert wrote: > * Steven Sistare (steven.sistare@oracle.com) wrote: >> On 7/30/2020 1:03 PM, Alex Bennée wrote: >>> >>> Steve Sistare <steven.sistare@oracle.com> writes: >>> >>>> Provide the -pause command-line parameter and the QEMU_PAUSE environment >>>> variable to briefly pause QEMU in main and allow a developer to attach gdb. >>>> Useful when the developer does not invoke QEMU directly, such as when using >>>> libvirt. >>> >>> How does this differ from -S? >> >> The -S flag runs qemu to the main loop but does not start the guest. Lots of code >> that you may need to debug runs before you get there. > > You might try the '--preconfig' option - that's pretty early on. > The other one is adding a chardev and telling it to wait for a server; > that'll wait until you telnet to the port. > > (Either way, this patch shouldn't really be part of this series, it's a > separate discussion) Sure, I will pull it from the series. - Steve >> - Steve >>>> Usage: >>>> qemu -pause <seconds> >>>> or >>>> export QEMU_PAUSE=<seconds> >>>> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>>> --- >>>> qemu-options.hx | 9 +++++++++ >>>> softmmu/vl.c | 15 ++++++++++++++- >>>> 2 files changed, 23 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/qemu-options.hx b/qemu-options.hx >>>> index 708583b..8505cf2 100644 >>>> --- a/qemu-options.hx >>>> +++ b/qemu-options.hx >>>> @@ -3668,6 +3668,15 @@ SRST >>>> option is experimental. >>>> ERST >>>> >>>> +DEF("pause", HAS_ARG, QEMU_OPTION_pause, \ >>>> + "-pause secs Pause for secs seconds on entry to main.\n", QEMU_ARCH_ALL) >>>> + >>>> +SRST >>>> +``--pause secs`` >>>> + Pause for a number of seconds on entry to main. Useful for attaching >>>> + a debugger after QEMU has been launched by some other entity. >>>> +ERST >>>> + >>> >>> It seems like having an option to race with the debugger is just asking >>> for trouble. >>> >>>> DEF("S", 0, QEMU_OPTION_S, \ >>>> "-S freeze CPU at startup (use 'c' to start execution)\n", >>>> QEMU_ARCH_ALL) >>>> diff --git a/softmmu/vl.c b/softmmu/vl.c >>>> index 8478778..951994f 100644 >>>> --- a/softmmu/vl.c >>>> +++ b/softmmu/vl.c >>>> @@ -2844,7 +2844,7 @@ static void create_default_memdev(MachineState *ms, const char *path) >>>> >>>> void qemu_init(int argc, char **argv, char **envp) >>>> { >>>> - int i; >>>> + int i, seconds; >>>> int snapshot, linux_boot; >>>> const char *initrd_filename; >>>> const char *kernel_filename, *kernel_cmdline; >>>> @@ -2882,6 +2882,13 @@ void qemu_init(int argc, char **argv, char **envp) >>>> QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list); >>>> int mem_prealloc = 0; /* force preallocation of physical target memory */ >>>> >>>> + if (getenv("QEMU_PAUSE")) { >>>> + seconds = atoi(getenv("QEMU_PAUSE")); >>>> + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", >>>> + seconds, getpid()); >>>> + sleep(seconds); >>>> + } >>>> + >>>> os_set_line_buffering(); >>>> >>>> error_init(argv[0]); >>>> @@ -3204,6 +3211,12 @@ void qemu_init(int argc, char **argv, char **envp) >>>> case QEMU_OPTION_gdb: >>>> add_device_config(DEV_GDB, optarg); >>>> break; >>>> + case QEMU_OPTION_pause: >>>> + seconds = atoi(optarg); >>>> + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", >>>> + seconds, getpid()); >>>> + sleep(seconds); >>>> + break; >>>> case QEMU_OPTION_L: >>>> if (is_help_option(optarg)) { >>>> list_data_dirs = true; >>> >>> >>
diff --git a/qemu-options.hx b/qemu-options.hx index 708583b..8505cf2 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3668,6 +3668,15 @@ SRST option is experimental. ERST +DEF("pause", HAS_ARG, QEMU_OPTION_pause, \ + "-pause secs Pause for secs seconds on entry to main.\n", QEMU_ARCH_ALL) + +SRST +``--pause secs`` + Pause for a number of seconds on entry to main. Useful for attaching + a debugger after QEMU has been launched by some other entity. +ERST + DEF("S", 0, QEMU_OPTION_S, \ "-S freeze CPU at startup (use 'c' to start execution)\n", QEMU_ARCH_ALL) diff --git a/softmmu/vl.c b/softmmu/vl.c index 8478778..951994f 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2844,7 +2844,7 @@ static void create_default_memdev(MachineState *ms, const char *path) void qemu_init(int argc, char **argv, char **envp) { - int i; + int i, seconds; int snapshot, linux_boot; const char *initrd_filename; const char *kernel_filename, *kernel_cmdline; @@ -2882,6 +2882,13 @@ void qemu_init(int argc, char **argv, char **envp) QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list); int mem_prealloc = 0; /* force preallocation of physical target memory */ + if (getenv("QEMU_PAUSE")) { + seconds = atoi(getenv("QEMU_PAUSE")); + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", + seconds, getpid()); + sleep(seconds); + } + os_set_line_buffering(); error_init(argv[0]); @@ -3204,6 +3211,12 @@ void qemu_init(int argc, char **argv, char **envp) case QEMU_OPTION_gdb: add_device_config(DEV_GDB, optarg); break; + case QEMU_OPTION_pause: + seconds = atoi(optarg); + printf("Pausing %d seconds for debugger. QEMU PID is %d\n", + seconds, getpid()); + sleep(seconds); + break; case QEMU_OPTION_L: if (is_help_option(optarg)) { list_data_dirs = true;
Provide the -pause command-line parameter and the QEMU_PAUSE environment variable to briefly pause QEMU in main and allow a developer to attach gdb. Useful when the developer does not invoke QEMU directly, such as when using libvirt. Usage: qemu -pause <seconds> or export QEMU_PAUSE=<seconds> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- qemu-options.hx | 9 +++++++++ softmmu/vl.c | 15 ++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-)