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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 7/5/19 3:19 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> 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é <philmd@redhat.com>
>>>>>>> ---
>>>>>>> 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.

I hope people concerned by security build QEMU with the NOP trace backend.

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

I understand. It would be clearer to have this position documented in
HACKING.

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

I'm fine with qtests. I was thinking on how sometimes I use ld.so's
LD_DEBUG env var, and thought it might be useful to use it a similar
way, but in a test environment (not restricted to qtests, but also
avocado tests and docker tests).
We can modify each type of test to use the '-trace' command line
argument. While it is a bit more of work, I understand the design
motivations to have a uniform interface.

Thanks,

Phil.