From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39375) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abUL7-0002fC-54 for qemu-devel@nongnu.org; Thu, 03 Mar 2016 09:34:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abUL3-0001gp-RS for qemu-devel@nongnu.org; Thu, 03 Mar 2016 09:34:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39161) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abUL3-0001gd-LP for qemu-devel@nongnu.org; Thu, 03 Mar 2016 09:34:49 -0500 References: <1457012886-7626-1-git-send-email-den@openvz.org> <56D840D4.4010109@redhat.com> <56D841C6.1060303@openvz.org> <56D84487.5070009@redhat.com> <56D84555.7090003@openvz.org> <56D8470C.5000201@redhat.com> <56D84955.6070508@openvz.org> From: Paolo Bonzini Message-ID: <56D84B85.70408@redhat.com> Date: Thu, 3 Mar 2016 15:34:45 +0100 MIME-Version: 1.0 In-Reply-To: <56D84955.6070508@openvz.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] log: fix hanged connect from virt-manager to libvirt List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org, Dimitris Aragiorgis 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