qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Richard Henderson
Subject: Re: [PULL for-7.1 08/36] *: Use fprintf between qemu_log_lock/unlock
Date: Fri, 25 Mar 2022 09:00:28 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0

On 3/24/22 08:30, Alex Bennée wrote:

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);
+    }
+}

If the lock succeeded, then we *know* that logfile != NULL -- indeed, that is exactly the test that is required following qemu_log_lock(). There is no point in structuring the code otherwise.

If you remove that, then you're left with

#define qemu_log__locked  fprintf

Can you tell me that's really an improvement?

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.

I can't see that. If we have a radical adjustment to logfiles that requires something other than fprintf, then we're probably going to change the type of logfile too. At which point all of the other existing places that we pass the FILE*, e.g. cpu_dump_state are also affected.


r~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]