* [PATCH v2] trace/simple: Fix unauthorized enable
@ 2020-05-27 6:56 Markus Armbruster
2020-05-27 7:38 ` Philippe Mathieu-Daudé
2020-06-17 15:26 ` Stefan Hajnoczi
0 siblings, 2 replies; 3+ messages in thread
From: Markus Armbruster @ 2020-05-27 6:56 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha
st_set_trace_file() accidentally enables tracing. It's called
unconditionally during startup, which is why QEMU built with the
simple trace backend always writes a trace file "trace-$PID".
This has been broken for quite a while. I didn't track down the exact
commit.
Fix st_set_trace_file() to restore the state.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
v2: Make st_set_trace_file() less confusing [Stefan]
trace/simple.h | 2 +-
trace/simple.c | 20 ++++++++++++++------
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/trace/simple.h b/trace/simple.h
index 5771a0634f..26ccbc8b8a 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -12,7 +12,7 @@
#define TRACE_SIMPLE_H
void st_print_trace_file_status(void);
-void st_set_trace_file_enabled(bool enable);
+bool st_set_trace_file_enabled(bool enable);
void st_set_trace_file(const char *file);
bool st_init(void);
void st_flush_trace_buffer(void);
diff --git a/trace/simple.c b/trace/simple.c
index fc7106ec49..9cd2ed1fb3 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -302,10 +302,17 @@ static int st_write_event_mapping(void)
return 0;
}
-void st_set_trace_file_enabled(bool enable)
+/**
+ * Enable / disable tracing, return whether it was enabled.
+ *
+ * @enable: enable if %true, else disable.
+ */
+bool st_set_trace_file_enabled(bool enable)
{
+ bool was_enabled = trace_fp;
+
if (enable == !!trace_fp) {
- return; /* no change */
+ return was_enabled; /* no change */
}
/* Halt trace writeout */
@@ -323,14 +330,14 @@ void st_set_trace_file_enabled(bool enable)
trace_fp = fopen(trace_file_name, "wb");
if (!trace_fp) {
- return;
+ return was_enabled;
}
if (fwrite(&header, sizeof header, 1, trace_fp) != 1 ||
st_write_event_mapping() < 0) {
fclose(trace_fp);
trace_fp = NULL;
- return;
+ return was_enabled;
}
/* Resume trace writeout */
@@ -340,6 +347,7 @@ void st_set_trace_file_enabled(bool enable)
fclose(trace_fp);
trace_fp = NULL;
}
+ return was_enabled;
}
/**
@@ -350,7 +358,7 @@ void st_set_trace_file_enabled(bool enable)
*/
void st_set_trace_file(const char *file)
{
- st_set_trace_file_enabled(false);
+ bool saved_enable = st_set_trace_file_enabled(false);
g_free(trace_file_name);
@@ -361,7 +369,7 @@ void st_set_trace_file(const char *file)
trace_file_name = g_strdup_printf("%s", file);
}
- st_set_trace_file_enabled(true);
+ st_set_trace_file_enabled(saved_enable);
}
void st_print_trace_file_status(void)
--
2.21.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] trace/simple: Fix unauthorized enable
2020-05-27 6:56 [PATCH v2] trace/simple: Fix unauthorized enable Markus Armbruster
@ 2020-05-27 7:38 ` Philippe Mathieu-Daudé
2020-06-17 15:26 ` Stefan Hajnoczi
1 sibling, 0 replies; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-27 7:38 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: stefanha
On 5/27/20 8:56 AM, Markus Armbruster wrote:
> st_set_trace_file() accidentally enables tracing. It's called
> unconditionally during startup, which is why QEMU built with the
> simple trace backend always writes a trace file "trace-$PID".
>
> This has been broken for quite a while. I didn't track down the exact
> commit.
>
> Fix st_set_trace_file() to restore the state.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> v2: Make st_set_trace_file() less confusing [Stefan]
>
> trace/simple.h | 2 +-
> trace/simple.c | 20 ++++++++++++++------
> 2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/trace/simple.h b/trace/simple.h
> index 5771a0634f..26ccbc8b8a 100644
> --- a/trace/simple.h
> +++ b/trace/simple.h
> @@ -12,7 +12,7 @@
> #define TRACE_SIMPLE_H
>
> void st_print_trace_file_status(void);
> -void st_set_trace_file_enabled(bool enable);
> +bool st_set_trace_file_enabled(bool enable);
> void st_set_trace_file(const char *file);
> bool st_init(void);
> void st_flush_trace_buffer(void);
> diff --git a/trace/simple.c b/trace/simple.c
> index fc7106ec49..9cd2ed1fb3 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -302,10 +302,17 @@ static int st_write_event_mapping(void)
> return 0;
> }
>
> -void st_set_trace_file_enabled(bool enable)
> +/**
> + * Enable / disable tracing, return whether it was enabled.
> + *
> + * @enable: enable if %true, else disable.
> + */
> +bool st_set_trace_file_enabled(bool enable)
> {
> + bool was_enabled = trace_fp;
> +
> if (enable == !!trace_fp) {
> - return; /* no change */
> + return was_enabled; /* no change */
> }
>
> /* Halt trace writeout */
> @@ -323,14 +330,14 @@ void st_set_trace_file_enabled(bool enable)
>
> trace_fp = fopen(trace_file_name, "wb");
> if (!trace_fp) {
> - return;
> + return was_enabled;
> }
>
> if (fwrite(&header, sizeof header, 1, trace_fp) != 1 ||
> st_write_event_mapping() < 0) {
> fclose(trace_fp);
> trace_fp = NULL;
> - return;
> + return was_enabled;
> }
>
> /* Resume trace writeout */
> @@ -340,6 +347,7 @@ void st_set_trace_file_enabled(bool enable)
> fclose(trace_fp);
> trace_fp = NULL;
> }
> + return was_enabled;
> }
>
> /**
> @@ -350,7 +358,7 @@ void st_set_trace_file_enabled(bool enable)
> */
> void st_set_trace_file(const char *file)
> {
> - st_set_trace_file_enabled(false);
> + bool saved_enable = st_set_trace_file_enabled(false);
>
> g_free(trace_file_name);
>
> @@ -361,7 +369,7 @@ void st_set_trace_file(const char *file)
> trace_file_name = g_strdup_printf("%s", file);
> }
>
> - st_set_trace_file_enabled(true);
> + st_set_trace_file_enabled(saved_enable);
> }
>
> void st_print_trace_file_status(void)
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] trace/simple: Fix unauthorized enable
2020-05-27 6:56 [PATCH v2] trace/simple: Fix unauthorized enable Markus Armbruster
2020-05-27 7:38 ` Philippe Mathieu-Daudé
@ 2020-06-17 15:26 ` Stefan Hajnoczi
1 sibling, 0 replies; 3+ messages in thread
From: Stefan Hajnoczi @ 2020-06-17 15:26 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, stefanha
[-- Attachment #1: Type: text/plain, Size: 761 bytes --]
On Wed, May 27, 2020 at 08:56:13AM +0200, Markus Armbruster wrote:
> st_set_trace_file() accidentally enables tracing. It's called
> unconditionally during startup, which is why QEMU built with the
> simple trace backend always writes a trace file "trace-$PID".
>
> This has been broken for quite a while. I didn't track down the exact
> commit.
>
> Fix st_set_trace_file() to restore the state.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> v2: Make st_set_trace_file() less confusing [Stefan]
>
> trace/simple.h | 2 +-
> trace/simple.c | 20 ++++++++++++++------
> 2 files changed, 15 insertions(+), 7 deletions(-)
Thanks, applied to my tracing tree:
https://github.com/stefanha/qemu/commits/tracing
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-17 15:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 6:56 [PATCH v2] trace/simple: Fix unauthorized enable Markus Armbruster
2020-05-27 7:38 ` Philippe Mathieu-Daudé
2020-06-17 15:26 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).