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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

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 <vilanova@ac.upc.edu>
> ---

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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]