On Fri, Jun 26, 2020 at 06:27:05PM +0200, Christophe de Dinechin wrote: Please run scripts/checkpatch.pl if you haven't already.
> diff --git a/configure b/configure
> index ae8737d5a2..130630b98f 100755
> --- a/configure
> +++ b/configure
> @@ -7702,6 +7702,11 @@ fi
> if have_backend "log"; then
> echo "CONFIG_TRACE_LOG=y" >> $config_host_mak
> fi
> +if have_backend "recorder"; then
> + echo "CONFIG_TRACE_RECORDER=y" >> $config_host_mak
> + # This is a bit brutal, but there is currently a bug in the makefiles
> + LIBS="$LIBS -lrecorder"
Outdated? Patch 1 was supposed to fix this.
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 60f395c276..565f518d4b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -297,6 +297,22 @@ ERST
> .cmd = hmp_trace_file,
> },
>
> +SRST
> +``trace-file on|off|flush``
> + Open, close, or flush the trace file. If no argument is given, the
> + status of the trace file is displayed.
> +ERST
> +#endif
> +
> +#if defined(CONFIG_TRACE_RECORDER)
> + {
> + .name = "recorder",
> + .args_type = "op:s?,arg:F?",
> + .params = "trace|dump [arg]",
> + .help = "trace selected recorders or print recorder dump",
> + .cmd = hmp_recorder,
> + },
> +
> SRST
> ``trace-file on|off|flush``
> Open, close, or flush the trace file. If no argument is given, the
This is the same trace-file command documentation that was added above.
Should this be the documentation for the recorder command?
> @@ -1120,7 +1136,7 @@ ERST
>
> SRST
> ``dump-guest-memory [-p]`` *filename* *begin* *length*
> - \
> + \
> ``dump-guest-memory [-z|-l|-s|-w]`` *filename*
> Dump guest memory to *protocol*. The file can be processed with crash or
> gdb. Without ``-z|-l|-s|-w``, the dump format is ELF.
Spurious change. Please drop.
> @@ -1828,4 +1844,3 @@ ERST
> .sub_table = hmp_info_cmds,
> .flags = "p",
> },
> -
Spurious change. Please drop.
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 89bb970b00..0094b1860f 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -61,6 +61,9 @@
> #ifdef CONFIG_TRACE_SIMPLE
> #include "trace/simple.h"
> #endif
> +#ifdef CONFIG_TRACE_RECORDER
> +#include "trace/recorder.h"
> +#endif
> #include "exec/memory.h"
> #include "exec/exec-all.h"
> #include "qemu/option.h"
> @@ -227,6 +230,30 @@ static void hmp_trace_file(Monitor *mon, const QDict
> *qdict)
> }
> #endif
>
> +#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 print it?
> diff --git a/trace/recorder.c b/trace/recorder.c
> new file mode 100644
> index 0000000000..cbc22ee2d5
> --- /dev/null
> +++ b/trace/recorder.c
> @@ -0,0 +1,22 @@
> +/*
> + * Recorder-based trace backend
> + *
> + * Copyright Red Hat 2020
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "trace/recorder.h"
> +
> +RECORDER_CONSTRUCTOR
> +void recorder_trace_init(void)
> +{
> + recorder_trace_set(getenv("RECORDER_TRACES"));
> +
> + // Allow a dump in case we receive some unhandled signal
> + // For example, send USR2 to a hung process to get a dump
CODING_STYLE.rst says:
We use traditional C-style /``*`` ``*``/ comments and avoid //
comments.
There are other instances in this patch series that I haven't pointed
out.
> + if (getenv("RECORDER_TRACES"))
> + recorder_dump_on_common_signals(0,0);
CODING_STYLE.rst says:
We use traditional C-style /``*`` ``*``/ comments and avoid //
comments.
> +}
> diff --git a/trace/recorder.h b/trace/recorder.h
> new file mode 100644
> index 0000000000..00b11a2d2f
> --- /dev/null
> +++ b/trace/recorder.h
> @@ -0,0 +1,34 @@
> +/*
> + * Recorder-based trace backend
> + *
> + * Copyright Red Hat 2020
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef TRACE_RECORDER_H
> +#define TRACE_RECORDER_H
> +
> +#include "qemu/osdep.h"
CODING_STYLE.rst says:
Do not include "qemu/osdep.h" from header files since the .c file will
have already included it.
Please either follow that or document the reason for the exception.
> +
> +#ifdef CONFIG_TRACE_RECORDER
Files including trace/recorder.h do this:
#ifdef CONFIG_TRACE_RECORDER
#include "trace/recorder.h"
#endif
Either CONFIG_TRACE_RECORDER should be checked before including the
header or inside the header, but doing both is confusing.
signature.asc
Description: PGP signature
