Re: [PULL for-7.1 08/36] *: Use fprintf between qemu_log_lock/unlock

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 3/23/22 10:22, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@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