From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53037) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RmTBt-0007Qo-QZ for qemu-devel@nongnu.org; Sun, 15 Jan 2012 11:44:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RmTBs-0007ph-3L for qemu-devel@nongnu.org; Sun, 15 Jan 2012 11:44:21 -0500 Received: from isrv.corpit.ru ([86.62.121.231]:34065) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RmTBr-0007p7-Os for qemu-devel@nongnu.org; Sun, 15 Jan 2012 11:44:20 -0500 Message-ID: <4F130261.4010205@msgid.tls.msk.ru> Date: Sun, 15 Jan 2012 20:44:17 +0400 From: Michael Tokarev MIME-Version: 1.0 References: <20120114124210.E43483DE@gandalf.tls.msk.ru> <4F12CB7C.308@msgid.tls.msk.ru> <4F12FAA7.5000103@redhat.com> In-Reply-To: <4F12FAA7.5000103@redhat.com> 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 20:11, Paolo Bonzini wrote: > 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. Daemon(3) on linux (glibc) does not try to fork twice, just one time is sufficient. Yes in old times there was some portability issues on some unixes with controling terminal and what not. That thread summaries it up almost nicely at the end: "So I suppose it all just boils down to tradition in the end - a single fork is sufficient as long as the parent dies in short order anyway," and "...think of the setsid( ) call as the "new" way to do thing (disassociate from the terminal) and the [second] fork( ) call after it as redundancy to deal with the SVr4..." [] >>>> + * 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. >>>> + /* 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. >> 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? 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. >>> 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. 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. Thanks, /mjt