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