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

[Christian Borntraeger]

On 10/17/2016 07:17 PM, Markus Armbruster wrote:
> Christian Borntraeger  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 
>> ---
>>  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.

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

[Markus Armbruster]

Christian Borntraeger  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 
> ---
>  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

[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

[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-borntrae...@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-de...@freelists.org

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 
>> ---
>>  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 
> ---
>  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
> 
>