[Qemu-devel] [PATCH/RFC] vl: add no-panic option

[Qemu-devel] [PATCH/RFC] vl: add no-panic option

[Christian Borntraeger]

Some testcase will trigger a guest panic state. For testing purposes
it can be useful to exit QEMU anyway.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 qemu-options.hx | 9 +++++++++
 vl.c            | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 01f01df..ee6d3d0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3301,6 +3301,15 @@ This allows for instance switching to monitor to commit changes to the
 disk image.
 ETEXI
 
+DEF("no-panic", 0, QEMU_OPTION_no_panic, \
+    "-no-panic       exit QEMU also in guest panic state\n", QEMU_ARCH_ALL)
+STEXI
+@item -no-panic
+@findex -no-panic
+Exit QEMU on guest panic instead of keeping it alive. This allows for
+instance running tests that are known to panic at the end.
+ETEXI
+
 DEF("loadvm", HAS_ARG, QEMU_OPTION_loadvm, \
     "-loadvm [tag|id]\n" \
     "                start right away with a saved state (loadvm in monitor)\n",
diff --git a/vl.c b/vl.c
index f3abd99..57e1d91 100644
--- a/vl.c
+++ b/vl.c
@@ -164,6 +164,7 @@ int no_hpet = 0;
 int fd_bootchk = 1;
 static int no_reboot;
 int no_shutdown = 0;
+int no_panic = 0;
 int cursor_hide = 1;
 int graphic_rotate = 0;
 const char *watchdog;
@@ -1774,6 +1775,8 @@ void qemu_system_reset(bool report)
 
 void qemu_system_guest_panicked(void)
 {
+    if (no_panic)
+	return qemu_system_shutdown_request();
     if (current_cpu) {
         current_cpu->crash_occurred = true;
     }
@@ -3780,6 +3783,9 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_no_shutdown:
                 no_shutdown = 1;
                 break;
+            case QEMU_OPTION_no_panic:
+                no_panic = 1;
+                break;
             case QEMU_OPTION_show_cursor:
                 cursor_hide = 0;
                 break;
-- 
2.5.5

Re: [Qemu-devel] [PATCH/RFC] vl: add no-panic option

[Paolo Bonzini]

> Some testcase will trigger a guest panic state. For testing purposes
> it can be useful to exit QEMU anyway.

I wonder if this should be done by default *unless* -no-shutdown is
provided.  This would require some planning (and delay this to 2.9,
in all likelihood), but it probably would be pretty nice for general
usage.

Paolo

> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  qemu-options.hx | 9 +++++++++
>  vl.c            | 6 ++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 01f01df..ee6d3d0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3301,6 +3301,15 @@ This allows for instance switching to monitor to
> commit changes to the
>  disk image.
>  ETEXI
>  
> +DEF("no-panic", 0, QEMU_OPTION_no_panic, \
> +    "-no-panic       exit QEMU also in guest panic state\n", QEMU_ARCH_ALL)
> +STEXI
> +@item -no-panic
> +@findex -no-panic
> +Exit QEMU on guest panic instead of keeping it alive. This allows for
> +instance running tests that are known to panic at the end.
> +ETEXI
> +
>  DEF("loadvm", HAS_ARG, QEMU_OPTION_loadvm, \
>      "-loadvm [tag|id]\n" \
>      "                start right away with a saved state (loadvm in
>      monitor)\n",
> diff --git a/vl.c b/vl.c
> index f3abd99..57e1d91 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -164,6 +164,7 @@ int no_hpet = 0;
>  int fd_bootchk = 1;
>  static int no_reboot;
>  int no_shutdown = 0;
> +int no_panic = 0;
>  int cursor_hide = 1;
>  int graphic_rotate = 0;
>  const char *watchdog;
> @@ -1774,6 +1775,8 @@ void qemu_system_reset(bool report)
>  
>  void qemu_system_guest_panicked(void)
>  {
> +    if (no_panic)
> +	return qemu_system_shutdown_request();
>      if (current_cpu) {
>          current_cpu->crash_occurred = true;
>      }
> @@ -3780,6 +3783,9 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_no_shutdown:
>                  no_shutdown = 1;
>                  break;
> +            case QEMU_OPTION_no_panic:
> +                no_panic = 1;
> +                break;
>              case QEMU_OPTION_show_cursor:
>                  cursor_hide = 0;
>                  break;
> --
> 2.5.5
> 
> 

Re: [Qemu-devel] [PATCH/RFC] vl: add no-panic option

[Christian Borntraeger]

On 10/17/2016 02:50 PM, Paolo Bonzini wrote:
>> Some testcase will trigger a guest panic state. For testing purposes
>> it can be useful to exit QEMU anyway.
> 
> I wonder if this should be done by default *unless* -no-shutdown is
> provided.  This would require some planning (and delay this to 2.9,
> in all likelihood), but it probably would be pretty nice for general
> usage.

Yes, might also an option. There are basically two cases
a: guest panic
b: qemu panic (e.g. if KVM_RUN return EFAULT)

I think for b, the current behaviour might be better. In any
case I want a tuneable and either -no-panic or the new -no-shutdown
would allow that.


> 
> Paolo
> 
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  qemu-options.hx | 9 +++++++++
>>  vl.c            | 6 ++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 01f01df..ee6d3d0 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3301,6 +3301,15 @@ This allows for instance switching to monitor to
>> commit changes to the
>>  disk image.
>>  ETEXI
>>  
>> +DEF("no-panic", 0, QEMU_OPTION_no_panic, \
>> +    "-no-panic       exit QEMU also in guest panic state\n", QEMU_ARCH_ALL)
>> +STEXI
>> +@item -no-panic
>> +@findex -no-panic
>> +Exit QEMU on guest panic instead of keeping it alive. This allows for
>> +instance running tests that are known to panic at the end.
>> +ETEXI
>> +
>>  DEF("loadvm", HAS_ARG, QEMU_OPTION_loadvm, \
>>      "-loadvm [tag|id]\n" \
>>      "                start right away with a saved state (loadvm in
>>      monitor)\n",
>> diff --git a/vl.c b/vl.c
>> index f3abd99..57e1d91 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -164,6 +164,7 @@ int no_hpet = 0;
>>  int fd_bootchk = 1;
>>  static int no_reboot;
>>  int no_shutdown = 0;
>> +int no_panic = 0;
>>  int cursor_hide = 1;
>>  int graphic_rotate = 0;
>>  const char *watchdog;
>> @@ -1774,6 +1775,8 @@ void qemu_system_reset(bool report)
>>  
>>  void qemu_system_guest_panicked(void)
>>  {
>> +    if (no_panic)
>> +	return qemu_system_shutdown_request();
>>      if (current_cpu) {
>>          current_cpu->crash_occurred = true;
>>      }
>> @@ -3780,6 +3783,9 @@ int main(int argc, char **argv, char **envp)
>>              case QEMU_OPTION_no_shutdown:
>>                  no_shutdown = 1;
>>                  break;
>> +            case QEMU_OPTION_no_panic:
>> +                no_panic = 1;
>> +                break;
>>              case QEMU_OPTION_show_cursor:
>>                  cursor_hide = 0;
>>                  break;
>> --
>> 2.5.5
>>
>>
> 

Re: [Qemu-devel] [PATCH/RFC] vl: add no-panic option

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH/RFC] vl: add no-panic option
Type: series
Message-id: 1476706440-112198-1-git-send-email-borntraeger@de.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
fe3696a vl: add no-panic option

=== OUTPUT BEGIN ===
Checking PATCH 1/1: vl: add no-panic option...
ERROR: do not initialise globals to 0 or NULL
#40: FILE: vl.c:167:
+int no_panic = 0;

ERROR: braces {} are necessary for all arms of this statement
#48: FILE: vl.c:1782:
+    if (no_panic)
[...]

ERROR: code indent should never use tabs
#49: FILE: vl.c:1783:
+^Ireturn qemu_system_shutdown_request();$

total: 3 errors, 0 warnings, 39 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH/RFC] vl: add no-panic option

[Paolo Bonzini]

On 17/10/2016 14:54, Christian Borntraeger wrote:
> On 10/17/2016 02:50 PM, Paolo Bonzini wrote:
>>> Some testcase will trigger a guest panic state. For testing purposes
>>> it can be useful to exit QEMU anyway.
>>
>> I wonder if this should be done by default *unless* -no-shutdown is
>> provided.  This would require some planning (and delay this to 2.9,
>> in all likelihood), but it probably would be pretty nice for general
>> usage.
> 
> Yes, might also an option. There are basically two cases
> a: guest panic
> b: qemu panic (e.g. if KVM_RUN return EFAULT)
> 
> I think for b, the current behaviour might be better.

(b) is not a guest panic, it's "INTERNAL_ERROR" right?  It would be easy
to accomodate the difference.  I tend to agree, since one may want to
play with the monitor in that case (e.g. x/10i $pc-20).

> In any
> case I want a tuneable and either -no-panic or the new -no-shutdown
> would allow that.

Let's change -no-shutdown then.  Actually I think we might even change
it in 2.8, since for example Libvirt always uses -no-shutdown and
everyone else that doesn't use it would probably hang on panics.

>>>  void qemu_system_guest_panicked(void)
>>>  {
>>> +    if (no_panic)
>>> +	return qemu_system_shutdown_request();
>>>      if (current_cpu) {
>>>          current_cpu->crash_occurred = true;
>>>      }

I think the "if (no_panic)" should go at the end so that the SHUTDOWN
event is sent after GUEST_PANICKED.

You would also have to add 'poweroff' to the GuestPanicAction enum too,
adjusting qemu_system_guest_panicked's call to
qapi_event_send_guest_panicked.

>>> @@ -3780,6 +3783,9 @@ int main(int argc, char **argv, char **envp)
>>>              case QEMU_OPTION_no_shutdown:
>>>                  no_shutdown = 1;
>>>                  break;
>>> +            case QEMU_OPTION_no_panic:
>>> +                no_panic = 1;
>>> +                break;
>>>              case QEMU_OPTION_show_cursor:
>>>                  cursor_hide = 0;
>>>                  break;
>>> --
>>> 2.5.5
>>>
>>>
>>
> 
> 
> 

Re: [Qemu-devel] [PATCH/RFC] vl: add no-panic option

[Markus Armbruster]

Christian Borntraeger <borntraeger@de.ibm.com> writes:

> Some testcase will trigger a guest panic state. For testing purposes
> it can be useful to exit QEMU anyway.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  qemu-options.hx | 9 +++++++++
>  vl.c            | 6 ++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 01f01df..ee6d3d0 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3301,6 +3301,15 @@ This allows for instance switching to monitor to commit changes to the
>  disk image.
>  ETEXI
>  
> +DEF("no-panic", 0, QEMU_OPTION_no_panic, \
> +    "-no-panic       exit QEMU also in guest panic state\n", QEMU_ARCH_ALL)
> +STEXI
> +@item -no-panic
> +@findex -no-panic
> +Exit QEMU on guest panic instead of keeping it alive. This allows for
> +instance running tests that are known to panic at the end.
> +ETEXI
> +
>  DEF("loadvm", HAS_ARG, QEMU_OPTION_loadvm, \
>      "-loadvm [tag|id]\n" \
>      "                start right away with a saved state (loadvm in monitor)\n",

Thank you for adding QEMU's 139-th option.  Are you sure it needs to be
an option of its own, and can't be added to an existing QemuOpts option
group?

Re: [Qemu-devel] [PATCH/RFC] vl: add no-panic option

[Christian Borntraeger]

On 10/17/2016 07:17 PM, Markus Armbruster wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> writes:
> 
>> Some testcase will trigger a guest panic state. For testing purposes
>> it can be useful to exit QEMU anyway.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  qemu-options.hx | 9 +++++++++
>>  vl.c            | 6 ++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 01f01df..ee6d3d0 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3301,6 +3301,15 @@ This allows for instance switching to monitor to commit changes to the
>>  disk image.
>>  ETEXI
>>  
>> +DEF("no-panic", 0, QEMU_OPTION_no_panic, \
>> +    "-no-panic       exit QEMU also in guest panic state\n", QEMU_ARCH_ALL)
>> +STEXI
>> +@item -no-panic
>> +@findex -no-panic
>> +Exit QEMU on guest panic instead of keeping it alive. This allows for
>> +instance running tests that are known to panic at the end.
>> +ETEXI
>> +
>>  DEF("loadvm", HAS_ARG, QEMU_OPTION_loadvm, \
>>      "-loadvm [tag|id]\n" \
>>      "                start right away with a saved state (loadvm in monitor)\n",
> 
> Thank you for adding QEMU's 139-th option.  Are you sure it needs to be
> an option of its own, and can't be added to an existing QemuOpts option
> group?

Your welcome, let me know if I should come up with more :-)
Just kidding, that is why I added RFC to the patch.

Paolo suggested to default to exit on panic unless 
-no-shutdown is given and I want to go that path.