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

[Richard Henderson <richard.henderson@linaro.org>]

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~