From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51567) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RmPXM-0007UA-Pq for qemu-devel@nongnu.org; Sun, 15 Jan 2012 07:50:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RmPXL-0000Z2-Nk for qemu-devel@nongnu.org; Sun, 15 Jan 2012 07:50:16 -0500 Received: from isrv.corpit.ru ([86.62.121.231]:35448) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RmPXL-0000Xf-GV for qemu-devel@nongnu.org; Sun, 15 Jan 2012 07:50:15 -0500 Message-ID: <4F12CB7C.308@msgid.tls.msk.ru> Date: Sun, 15 Jan 2012 16:50:04 +0400 From: Michael Tokarev MIME-Version: 1.0 References: <20120114124210.E43483DE@gandalf.tls.msk.ru> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 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: Paolo Bonzini Cc: qemu-devel@nongnu.org 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. >> - >> - /* ... 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. >> + /* 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 the place 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. > 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). Thanks, /mjt