From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RmTvg-0001Ix-CQ for qemu-devel@nongnu.org; Sun, 15 Jan 2012 12:31:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RmTvf-0006Yp-Bf for qemu-devel@nongnu.org; Sun, 15 Jan 2012 12:31:40 -0500 Received: from mail-ey0-f173.google.com ([209.85.215.173]:46395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RmTvf-0006Yl-2L for qemu-devel@nongnu.org; Sun, 15 Jan 2012 12:31:39 -0500 Received: by eaan12 with SMTP id n12so830513eaa.4 for ; Sun, 15 Jan 2012 09:31:38 -0800 (PST) Sender: Paolo Bonzini Message-ID: <4F130D78.1070903@redhat.com> Date: Sun, 15 Jan 2012 18:31:36 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <20120114124210.E43483DE@gandalf.tls.msk.ru> <4F12CB7C.308@msgid.tls.msk.ru> <4F12FAA7.5000103@redhat.com> <4F130261.4010205@msgid.tls.msk.ru> In-Reply-To: <4F130261.4010205@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: qemu-devel@nongnu.org On 01/15/2012 05:44 PM, Michael Tokarev wrote: >>>>> + * stdout (temporarily) to the pipe to parent, >>>> >>>> This is a bit of a hack. >>> >>> There's another way -- to keep the writing pipe end in some >>> local variable and use that one instead of STDOUT_FILENO. >>> I can do it that way for sure, just thought it's already >>> using too much local variables. >> >> Yes, that would be better. > > Done in a v2 version I sent you. Please stay on the list. >>>>> + /* now complete the daemonizing procedure. >>>>> + */ >>>>> + if (device&& !verbose) { >>>>> + if (chdir("/")< 0) { >>>>> + err(EXIT_FAILURE, "unable to chdir to /"); >>>>> + } >>>>> + /* this redirects stderr to /dev/null */ >>>>> + dup2(STDIN_FILENO, STDERR_FILENO); >>>>> + /* this redirects stdout to /dev/null too, and closes parent pipe */ >>>>> + dup2(STDIN_FILENO, STDOUT_FILENO); >>>>> + } >>>>> + >>>> >>>> Half of this is already done in client_thread, and that would be >>>> theplace where you should add dup2(0, 1). > > Um, I missed that "half of this" part. Indeed, nbd_client_thread() > does dup2(STDOUT_FILENO, STDERR_FILENO) which should go away, but > it is harmless for now, and can be addressed in a separate patch. Again, _the client thread_ is the right place to do this! See below. >>> I partly disagree. >>> >>> I wanted to de-couple -c (device) case with daemonizing. >>> client_thread only works in -c case, but daemonizing in >>> that case is wrong as I already pointed out in another >>> email - we should either stop daemonizing here at all >>> or have a separate option for it. >> >> We can only clean up standard file descriptors after all >> initialization tasks have been done. nbd_client_thread could still write >> error messages. Your patch introduces a race. > > Please elaborate where the race is. Do you mean one > thread can write error message while another at the > same time is closing the filedescriptor in question, -- > that race? Yes. > We're doomed anyway, and it is even good > we've a small remote chance for our error message to > be seen. Currently it just goes to /dev/null. No, currently it is sent from the daemon to the parent through the pipe, the parent prints it and exits with status code 1. With your patch, if the dup2 wins the race you exit with status code 0; if the client thread wins the race it is the same as master. > That's not a bad intention. I'm fixing existing logic without > introducing new logical changes. If you want to fix other > stuff, it is better be done in a separate commit/change. AFAIK the only known bug (besides the devfd/sockfd mixup) is the missing chdir, and that should be fixed first. Paolo