Robert Foley <robert.fo...@linaro.org> writes:
> On Tue, 12 Nov 2019 at 12:36, Alex Bennée <alex.ben...@linaro.org> wrote:
>>
>>
>> > }
>> > + atomic_rcu_set(&qemu_logfile, logfile);
>> > }
>> > - qemu_mutex_unlock(&qemu_logfile_mutex);
>> > + logfile = qemu_logfile;
>>
>> Isn't this read outside of the protection of both rcu_read_lock() and
>> the mutex? Could that race?
>
> This assignment is under the mutex. This change moved the mutex
> unlock towards the end of the function, just a few lines below the
> call_rcu().
Ahh I see now, I went +- blind ;-)
>
>> > +
>> > if (qemu_logfile &&
>> > (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) {
>> > - qemu_log_close();
>> > + atomic_rcu_set(&qemu_logfile, NULL);
>> > + call_rcu(logfile, qemu_logfile_free, rcu);
>>
>> I wonder if we can re-arrange the logic here so it's lets confusing? For
>> example the NULL qemu_loglevel can be detected at the start and we
>> should be able just to squash the current log and reset and go. I'm not
>> sure about the damonize/stdout check.
>>
>> > }
>> > + qemu_mutex_unlock(&qemu_logfile_mutex);
>> > }
>> >
>
> Absolutely, the logic that was here originally can be improved. We
> found it confusing also.
> We could move things around a bit and change it to something like this
> to help clarify.
>
> bool need_to_open_file = false;
> /*
> * In all cases we only log if qemu_loglevel is set.
> * And:
> * If not daemonized we will always log either to stderr
> * or to a file (if there is a logfilename).
> * If we are daemonized,
> * we will only log if there is a logfilename.
> */
> if (qemu_loglevel && (!is_daemonized() || logfilename) {
> need_to_open_file = true;
> }
> g_assert(qemu_logfile_mutex.initialized);
> qemu_mutex_lock(&qemu_logfile_mutex);
>
> if(qemu_logfile && !need_to_open_file) {
> /* Close file. */
> QemuLogFile *logfile = qemu_logfile;
> atomic_rcu_set(&qemu_logfile, NULL);
> call_rcu(logfile, qemu_logfile_free, rcu);
> } else if(!qemu_logfile && need_to_open_file) {
> logfile = g_new0(QemuLogFile, 1);
> __snip__ existing patch logic for opening the qemu_logfile will
> be inserted here.
> }
> qemu_mutex_unlock(&qemu_logfile_mutex);
That reads a lot better thanks. It's probably worth doing the clean-up in
a separate patch so it is easier to see the mutex's when added.
--
Alex Bennée
