Re: [Qemu-devel] [RFC PATCH-for-4.2] tracing: Allow to tune tracing options via the environment

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> writes:

> On 7/5/19 10:07 AM, Stefan Hajnoczi wrote:
>> On Thu, Jul 04, 2019 at 11:28:37AM +0100, Daniel P. Berrangé wrote:
>>> On Thu, Jul 04, 2019 at 11:24:57AM +0100, Stefan Hajnoczi wrote:
>>>> On Wed, Jul 03, 2019 at 06:25:01PM +0100, Daniel P. Berrangé wrote:
>>>>> On Wed, Jul 03, 2019 at 07:10:05PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>> We can pass trace trace options with the -trace command line
>>>>>> argument.
>>>>>> 
>>>>>> Tracing might be useful when running qtests. To avoid to have
>>>>>> to modify the tests and recompile, add the possibility to pass
>>>>>> trace options via the shell environment.

Unless I'm missing something, you don't have to recompile to pass
additional options.  The qtest binaries already read environment
variables QTEST_QEMU_BINARY and QTEST_QEMU_IMG.  Have them point to
suitable wrappers.

Perhaps there's still a need for a more convenient way.

>>>>>> 
>>>>>> We add:
>>>>>> - QEMU_TRACE_EVENTS:    List of events to enable (coma separated)
>>>>>> - QEMU_TRACE_EVENTFILE: File with list of events to enable
>>>>>> - QEMU_TRACE_LOGFILE:   File to log the trace events.
>>>>>> 
>>>>>> Example of use:
>>>>>> 
>>>>>>   $ QEMU_TRACE_EVENTS=pl011\* make check-qtest-arm
>>>>>>     TEST    check-qtest-arm: tests/boot-serial-test
>>>>>>   18650@1562168430.027490:pl011_can_receive LCR 0x00000000 read_count 0 
>>>>>> returning 1
>>>>>>   18650@1562168430.027535:pl011_can_receive LCR 0x00000000 read_count 0 
>>>>>> returning 1
>>>>>>   18650@1562168430.027544:pl011_can_receive LCR 0x00000000 read_count 0 
>>>>>> returning 1
>>>>>>   18650@1562168430.028037:pl011_can_receive LCR 0x00000000 read_count 0 
>>>>>> returning 1
>>>>>>   18650@1562168430.028049:pl011_can_receive LCR 0x00000000 read_count 0 
>>>>>> returning 1
>>>>>>   18650@1562168430.028057:pl011_can_receive LCR 0x00000000 read_count 0 
>>>>>> returning 1
>>>>>>   18653@1562168430.053250:pl011_write addr 0x00000000 value 0x00000054
>>>>>>   18653@1562168430.053276:pl011_irq_state irq state 0
>>>>>>   [...]
>>>>>> 
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>>>>> ---
>>>>>> I'm not sure where to document that...
>>>>>> ---
>>>>>>  trace/control.c | 9 +++++++++
>>>>>>  1 file changed, 9 insertions(+)
>>>>>> 
>>>>>> diff --git a/trace/control.c b/trace/control.c
>>>>>> index 43fb7868db..aea802623c 100644
>>>>>> --- a/trace/control.c
>>>>>> +++ b/trace/control.c
>>>>>> @@ -288,6 +288,8 @@ void trace_fini_vcpu(CPUState *vcpu)
>>>>>>  
>>>>>>  bool trace_init_backends(void)
>>>>>>  {
>>>>>> +    char *trace_env;
>>>>>> +
>>>>>>  #ifdef CONFIG_TRACE_SIMPLE
>>>>>>      if (!st_init()) {
>>>>>>          fprintf(stderr, "failed to initialize simple tracing 
>>>>>> backend.\n");
>>>>>> @@ -306,6 +308,13 @@ bool trace_init_backends(void)
>>>>>>      openlog(NULL, LOG_PID, LOG_DAEMON);
>>>>>>  #endif
>>>>>>  
>>>>>> +    trace_init_file(getenv("QEMU_TRACE_LOGFILE"));
>>>>>> +    trace_init_events(getenv("QEMU_TRACE_EVENTFILE"));
>>>>>> +    trace_env = getenv("QEMU_TRACE_EVENTS");
>>>>>> +    if (trace_env) {
>>>>>> +        trace_enable_events(trace_env);
>>>>>> +    }
>>>>>> +
>>>>>
>>>>> I don't think it is a nice idea to add this via environment variables
>>>>> to QEMU itself. Why not modify libqtest qtest_init_without_qmp_handshake
>>>>> to read the env vars and then pass a suitable -trace arg when spawning
>>>>> QEMU ?
>>>>
>>>> What is the concern about adding these environment variables to QEMU?
>>>>
>>>> It is convenient to be able to use tracing even if QEMU is invoked by
>>>> something you cannot modify/control.
>>>>
>>>> The main issues I see with environment variables are:
>>>>
>>>> 1. Security.  Is there a scenario where an attacker can use environment
>>>>    variables to influence the behavior of a QEMU process running at a
>>>>    different trust level?

The common (and sad) solution for this is to require whatever runs $PROG
at a different trust level to scrub the environment.

>>>> 2. Name collision.  What is the chance that existing users already
>>>>    define environment variables with these names and that unexpected
>>>>    behavior could result?
>>>
>>> One of the biggest problems with QEMU in general has been poorly modelled
>>> & defined interfaces for configuration. At runtime we've solved this with
>>> QMP. At startup we're still fighting the horror of QemuOpts in general and
>>> haven't got startup modelling to be on a par with that offered by QEMU.
>>> It was even worse when Audio didn't even use QemuOpts and instead used
>>> an arbitrary set of poorly defined env variables. To me adding yet another
>>> way to configure QEMU via env vars is moving in the opposite direction to
>>> what we want.
>> 
>> In this case the environment variables are optional and meant for cases
>> where the user cannot change the QEMU command-line.  I think they serve
>> a different purpose from the audio subsystem environment variables and
>> I'd be happy to merge them.

You're right, there is a difference between "also" and "only".  Audio
could only be configured via environment, and that was without doubt
awful.  Does not imply permitting trace configuration via environment
also would be similarly awful.

However, adding special cases as needed is what got us into the startup
mess Dan highlighted.  Are we confident tracing will remain the only
thing we also want to configure via environment?

I sympathize with Dan's plea for more uniform configuration interfaces.

>> Philippe: Have you tried adding the environment variable to libqtest as
>> Dan suggested and did it work for your use case?

This would fit into our existing use of the environment with qtest.  Try
grep '"QTEST_'.

> Yes, but we loose the ability to use this feature from linux-user and
> other tools:
>
> $ git grep trace_opt_parse
> bsd-user/main.c:851:            trace_file = trace_opt_parse(optarg);
> linux-user/main.c:387:    trace_file = trace_opt_parse(arg);
> qemu-img.c:5063:            trace_file = trace_opt_parse(optarg);
> qemu-io.c:579:            trace_file = trace_opt_parse(optarg);
> qemu-nbd.c:862:            trace_file = trace_opt_parse(optarg);
> scsi/qemu-pr-helper.c:969:            trace_file = trace_opt_parse(optarg);
> vl.c:3730:                trace_file = trace_opt_parse(optarg);
>
> So I'm now mixed about the trade off regarding Daniel worries.

Philippe, your commit message mentions just qtest.  That use case would
be covered by Dan's suggestion, wouldn't it?  If not, please explain.
If you have additional use cases, please state them.