Re: [Qemu-devel] [PATCH 1/1] log: fix hanged connect from virt-manager to libvirt

["Denis V. Lunev" <den@openvz.org>]

On 03/03/2016 05:34 PM, Paolo Bonzini wrote:
>
> On 03/03/2016 15:25, Denis V. Lunev wrote:
>>> Actually, the patch in v1 is fine.  My worry after looking at your patch
>>> was that I didn't have the dup2(stdout, stderr) case.  However, with my
>>> change you can never call qemu_log_close if is_daemonized(), because
>>> even the monitor command "logfile" cannot set logfilename to NULL.
>> IMHO you are wrong.
> Possible. :)
>
>> void qemu_set_log_filename(const char *filename)
>> {
>>      g_free(logfilename);
>>      logfilename = g_strdup(filename);
>>      qemu_log_close();
>>      qemu_set_log(qemu_loglevel);
>> }
>>
>> static void hmp_logfile(Monitor *mon, const QDict *qdict)
>> {
>>      qemu_set_log_filename(qdict_get_str(qdict, "filename"));
>> }
>>
>> This means that we will have qemu_log_close()
>> called in ANY case, even daemonized.
>>  From my point of view stderr will continue to be mapped
>> to the old file if we request to stop logging either by
>> zero mask or by setting empty filename.
> Yes, but filename will never be NULL in hmp_logfile, will it?  It's
> declared as 'F' in hmp-commands.hx, not as 'F?'.  If this is changed, I
> agree that the dup2 needs to be added.
>
> A different issue is that do_qemu_set_log *exits* instead of printing an
> Error if fopen fails. In my opinion, in this case logging should not
> even be disabled in this case.  But it can and should be fixed separately.
>
> The code is really ugly, it is old and used to be just a debugging aid.
>
> Paolo
ok, you are finally right. Your original code is correct and will
work even in these corner case.

Will it make sense to send a patch which will replace that code
to my one as followup + fix if 'fopen' will fail?

Den