Richard Henderson <richard.hender...@linaro.org> writes:
> On 3/23/22 10:22, Alex Bennée wrote:
>> Richard Henderson <richard.hender...@linaro.org> writes:
>>
>>> Inside qemu_log, we perform qemu_log_lock/unlock, which need
>>> not be done if we have already performed the lock beforehand.
>>>
>>> Always check the result of qemu_log_lock -- only checking
>>> qemu_loglevel_mask races with the acquisition of the lock
>>> on the logfile.
>> I'm not sure I like introducing all these raw fprintfs over
>> introducing
>> a function like qemu_log__locked().
>
> There's no way to implement qemu_log__locked with rcu. The lookup
> itself is what needs the locking; the return value of the FILE* is
> then valid until the unlock. To lookup the FILE* again would require
> more atomic operations.
That's not what I'm suggesting. qemu_log__locked would be a fairly
simple wrapper around the fprintf:
modified include/qemu/log.h
@@ -70,6 +70,25 @@ void qemu_log_unlock(FILE *fd);
} \
} while (0)
+/**
+ * qemu_log__locked() - log to a locked file
+ * @logfile: FILE handle from qemu_log_lock()
+ * @fmt: printf format
+ * ...: varargs
+ */
+static inline void G_GNUC_PRINTF(2, 3)
+ qemu_log__locked(FILE *logfile, const char *fmt, ...)
+{
+ if (logfile) {
+ va_list ap;
+
+ va_start(ap, fmt);
+ vfprintf(logfile, fmt, ap);
+ va_end(ap);
+ }
+}
+
+
/* Maintenance: */
/* define log items */
modified accel/tcg/translate-all.c
@@ -1546,10 +1546,10 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
}
/* Dump header and the first instruction */
- fprintf(logfile, "OUT: [size=%d]\n", gen_code_size);
- fprintf(logfile,
- " -- guest addr 0x" TARGET_FMT_lx " + tb prologue\n",
- tcg_ctx->gen_insn_data[insn][0]);
+ qemu_log__locked(logfile, "OUT: [size=%d]\n", gen_code_size);
+ qemu_log__locked(logfile,
+ " -- guest addr 0x" TARGET_FMT_lx " + tb
prologue\n",
+ tcg_ctx->gen_insn_data[insn][0]);
chunk_start = tcg_ctx->gen_insn_end_off[insn];
disas(logfile, tb->tc.ptr, chunk_start);
@@ -1561,8 +1561,8 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
while (insn < tb->icount) {
size_t chunk_end = tcg_ctx->gen_insn_end_off[insn];
if (chunk_end > chunk_start) {
- fprintf(logfile, " -- guest addr 0x" TARGET_FMT_lx "\n",
- tcg_ctx->gen_insn_data[insn][0]);
+ qemu_log__locked(logfile, " -- guest addr 0x"
TARGET_FMT_lx "\n",
+ tcg_ctx->gen_insn_data[insn][0]);
disas(logfile, tb->tc.ptr + chunk_start,
chunk_end - chunk_start);
I would home the inline would mean the compiler could do a semi-decent
job of eliding the multiple logfile checks. The _locked suffix is simply
to indicate it expects a pre-locked file.
> And I do prefer the printfs over repeated qemu_log.
The main benefit from my point of view is it keeps qemu_log operations
grouped together and easier to fix if we for example want to tweak how
we deal with log files in the future.
>
>
> r~
--
Alex Bennée
