From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53684) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RmSg1-00074w-JA for qemu-devel@nongnu.org; Sun, 15 Jan 2012 11:11:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RmSg0-0003Br-8s for qemu-devel@nongnu.org; Sun, 15 Jan 2012 11:11:25 -0500 Received: from mail-ee0-f45.google.com ([74.125.83.45]:54296) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RmSfz-0003Bl-UR for qemu-devel@nongnu.org; Sun, 15 Jan 2012 11:11:24 -0500 Received: by eekc41 with SMTP id c41so1637111eek.4 for ; Sun, 15 Jan 2012 08:11:22 -0800 (PST) Sender: Paolo Bonzini Message-ID: <4F12FAA7.5000103@redhat.com> Date: Sun, 15 Jan 2012 17:11:19 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <20120114124210.E43483DE@gandalf.tls.msk.ru> <4F12CB7C.308@msgid.tls.msk.ru> In-Reply-To: <4F12CB7C.308@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 01:50 PM, Michael Tokarev wrote: > On 15.01.2012 14:42, Paolo Bonzini wrote: >> On 01/14/2012 01:39 PM, Michael Tokarev wrote: >>> if (pid == 0) { >>> - close(stderr_fd[0]); >>> - ret = qemu_daemon(0, 0); >>> - >>> - /* Temporarily redirect stderr to the parent's pipe... */ >>> - dup2(stderr_fd[1], STDERR_FILENO); >>> - if (ret == -1) { >>> + int nullfd = open("/dev/null", O_RDWR); >>> + if (nullfd< 0 || setsid()< 0) { >>> err(EXIT_FAILURE, "Failed to daemonize"); >>> } >> >> This is forking only once. > > Is it good or bad? There's no need to fork twice. Second > fork (to the one which is already done in daemon(3)) has > been done to work around lack of proper communication between > parent and child in case of using plain daemon(3). I.e., due > to daemon(3) interface being unflexible/unsuitable for the > current use case. daemon(3) forks twice (so qemu-nbd is effectively forking three times, one of which is unnecessary). See http://stackoverflow.com/questions/881388/what-is-the-reason-for-performing-a-double-fork-when-creating-a-daemon for why there is a fork before setsid and one after. >>> - >>> - /* ... close the descriptor we inherited and go on. */ >>> - close(stderr_fd[1]); >>> - } else { >>> - bool errors = false; >>> - char *buf; >>> - >>> - /* In the parent. Print error messages from the child until >>> - * it closes the pipe. >>> + /* redirect stdin from /dev/null, >>> + * 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. >>> + /* 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). > > 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. >> Also, the chdir can be moved earlier, after bdrv_open. > > There's no need to, afiacs. We complete init process and > enter main loop. Chdir should be done befor entering main > loop, the rest makes no difference (as long as the files > we open will be accessible from cwd). Yes, but I prefer to have the chdir done unconditionally as soon as possible. Paolo