Re: [PATCH v4 1/2] trace: Add support for recorder back-end

[Stefan Hajnoczi]

On Thu, Jul 23, 2020 at 03:29:02PM +0200, Christophe de Dinechin wrote:
> diff --git a/configure b/configure
> index 4bd80ed507..3770ff873d 100755
> --- a/configure
> +++ b/configure
> @@ -7761,6 +7761,20 @@ fi
>  if have_backend "log"; then
>    echo "CONFIG_TRACE_LOG=y" >> $config_host_mak
>  fi
> +if have_backend "recorder"; then
> +    recorder_minver="1.0.10"
> +    if $pkg_config --atleast-version=$recorder_minver recorder ; then
> +        recorder_cflags="$($pkg_config --cflags recorder)"
> +        recorder_libs="$($pkg_config --libs recorder)"
> +        LIBS="$recorder_libs $LIBS"
> +        libs_qga="$recorder_libs $libs_qga"
> +        QEMU_CFLAGS="$QEMU_CFLAGS $recorder_cflags"
> +        zstd="yes"

Why enable zstd?

This happens after the zstd library dependency probe, so it overrides
the zstd setting even if the probe failed earlier on. This seems
strange.

> +#if defined(CONFIG_TRACE_RECORDER)
> +    {
> +        .name       = "recorder",
> +        .args_type  = "op:s?,arg:s?",
> +        .params     = "trace|dump [arg]",
> +        .help       = "trace selected recorders or print recorder dump",
> +        .cmd        = hmp_recorder,
> +    },
> +
> +SRST
> +``trace`` *cmds*

How is this sub-command different from the "trace-event <name> on|off"
HMP command?

> +  Activate or deactivate tracing for individual recorder traces.
> +  See recorder_trace_set(3) for the syntax of *cmds*
> +  For example, to activate trace ``foo`` and disable alll traces

s/alll/all/

> +  ending in ``_warning``, use ``foo:.*_warning=0``.
> +  Using ``help`` will list available traces and their current setting.

"help" is not listed in .params.

How is this sub-command different from the "info trace-events" HMP
command?

> +#ifdef CONFIG_TRACE_RECORDER
> +static void hmp_recorder(Monitor *mon, const QDict *qdict)
> +{
> +    const char *op = qdict_get_try_str(qdict, "op");
> +    const char *arg = qdict_get_try_str(qdict, "arg");
> +
> +    if (!op) {
> +        monitor_printf(mon, "missing recorder command\"%s\"\n", op);

op is NULL, why format it?

There is a space missing after "command".

> +def generate_c_begin(events, group):
> +    out('#include "qemu/osdep.h"',
> +        '#include "trace/control.h"',
> +        '#include "trace/simple.h"',

Is this header needed?

> +RECORDER_CONSTRUCTOR
> +void recorder_trace_init(void)

This function is called 3 times:
1. RECORDER_CONSTRUCTOR
2. trace_init_backends()
3. module_load_file()

That is unusual for an "init" function. Are all 3 really necessary?
Please add a comment explaining what is going on here and/or rename it
to recorder_trace_reinit().

> +{
> +    const char *traces = getenv("RECORDER_TRACES");
> +    recorder_trace_set(traces);
> +
> +    /*
> +     * Allow a dump in case we receive some unhandled signal
> +     * For example, send USR2 to a hung process to get a dump
> +     */
> +    if (traces) {
> +        recorder_dump_on_common_signals(0, 0);

Did you check all programs (i.e. qemu-system-*, qemu-user-*, qemu-img,
etc) to see whether installing various signal handlers is okay?

The library documentation says the following signals are handled:

  SIGQUIT, SIGILL, SIGABRT, SIGBUS, SIGSEGV, SIGSYS, SIGXCPU, SIGXFSZ,
  SIGINFO, SIGUSR1, SIGUSR2, SIGSTKFLT, SIGPWR

QEMU uses SIGBUS and SIGUSR1 ("SIG_IPI"). I'm not sure of the signal
policy for qemu-user-* but you might need to take special precautions.

I don't think installing signal handlers from an
__attribute__((constructor)) function is a viable approach since there
is no control over the ordering. If other object files do the same then
whose signal handler ends up being installed?

It may be cleaner to have a separate function called
recorder_setup_signals() that the programs can call when they install
signal handlers. That way the recorder signal handlers are installed
along with all the other signal handlers at program startup instead of
being installed from a constructor function that is hard to find.

signature.asc
Description: PGP signature