Re: [Qemu-devel] [PATCH v2 17/23] instrument: Add commandline options to start with an instrumentation library

[Lluís Vilanova]

Eric Blake writes:

> On 04/16/2013 07:51 AM, Lluís Vilanova wrote:
>> Add commandline options to control initial loading of dynamic instrumentation
>> library.
>> 
>> Signed-off-by: Lluís Vilanova <address@hidden>
>> ---

>> @@ -688,6 +690,15 @@ static void usage(void)
>> #endif
>> "-bsd type         select emulated BSD type FreeBSD/NetBSD/OpenBSD 
>> (default)\n"
>> "\n"
>> +#if defined(CONFIG_TRACE_INSTRUMENT)
>> +           "Tracing options:\n"
>> +#if defined(CONFIG_TRACE_INSTRUMENT_DYNAMIC)
>> +           "-instr path       load a dynamic trace instrumentation 
>> library\n"
>> +#endif
>> +           "-instr-arg string\n"
>> +           "                  argument to dynamic trace instrumentation 
>> library (can be given multiple times)\n"
>> +           "\n"

> Why do you document -instr-arg unconditionally, but -instr only when
> dynamic trace support is enabled; especially since the text of
> -instr-arg mentions that it is tied to dynamic tracing?

>> @@ -852,6 +866,14 @@ int main(int argc, char **argv)
>> singlestep = 1;
>> } else if (!strcmp(r, "strace")) {
>> do_strace = 1;
>> +#if defined(CONFIG_TRACE_INSTRUMENT_DYNAMIC)
>> +        } else if (!strcmp(r, "instr")) {
>> +            instrument_path = argv[optind++];
>> +#endif
>> +#if defined(CONFIG_TRACE_INSTRUMENT)
>> +        } else if (!strcmp(r, "instr-arg")) {
>> +            instr_parse_args(argv[optind++], &instrument_argc, 
>> &instrument_argv);
>> +#endif

> At least your parsing code matches your documentation, but it still
> feels weird.

You cannot load a library in non-dynamic mode, but you can still pass it some
arguments if it's in static mode (CONFIG_TRACE_INSTRUMENT accounts for all
cases), thus the "weirdness" :)


>> @@ -3378,6 +3397,14 @@ static const struct qemu_argument arg_table[] = {
>> {"R",          "QEMU_RESERVED_VA", true,  handle_arg_reserved_va,
>> "size",       "reserve 'size' bytes for guest virtual address space"},
>> #endif
>> +#if defined(CONFIG_TRACE_INSTRUMENT_DYNAMIC)
>> +    {"instr",      "QEMU_INSTR",       true,  handle_arg_instrument,
>> +     "path",       "load a dynamic trace instrumentation library"},
>> +#endif
>> +#if defined(CONFIG_TRACE_INSTRUMENT)
>> +    {"instr-arg",  "QEMU_INSTR_ARGS", true,  handle_arg_instrument_arg,
>> +     "string",     "argument to dynamic trace instrumentation library (can 
>> be given multiple times)"},

> Another case of mentioning the term dynamic even when dynamic
> instrumentation is not enabled.

Fixed.


Thanks a lot,
  Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth