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

[Philippe Mathieu-Daudé]

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:
>>>>> @@ -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?
>>>
>>> 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.
> 
> Philippe: Have you tried adding the environment variable to libqtest as
> Dan suggested and did it work for your use case?

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.

signature.asc
Description: OpenPGP digital signature