From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45955) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xhag4-000877-Ro for qemu-devel@nongnu.org; Fri, 24 Oct 2014 04:57:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xhag0-0001mf-1p for qemu-devel@nongnu.org; Fri, 24 Oct 2014 04:56:56 -0400 Message-ID: <544A1431.9050109@huawei.com> Date: Fri, 24 Oct 2014 16:56:17 +0800 From: Gonglei MIME-Version: 1.0 References: <1411638384-5844-1-git-send-email-arei.gonglei@huawei.com> <1411638384-5844-3-git-send-email-arei.gonglei@huawei.com> <544A007F.2090806@msgid.tls.msk.ru> In-Reply-To: <544A007F.2090806@msgid.tls.msk.ru> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/4] os-posix: report error message when lock file failed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: "peter.maydell@linaro.org" , "Huangweidong (C)" , "qemu-trivial@nongnu.org" , Luonengjun , "qemu-devel@nongnu.org" , "armbru@redhat.com" , "pbonzini@redhat.com" , "Huangpeng (Peter)" On 2014/10/24 15:32, Michael Tokarev wrote: > On 09/25/2014 01:46 PM, arei.gonglei@huawei.com wrote: >> From: Gonglei >> >> It will cause that create vm failed When manager >> tool is killed forcibly (kill -9 libvirtd_pid), >> the file not was unlink, and unlock. It's better >> that report the error message for users. >> >> Signed-off-by: Huangweidong >> Signed-off-by: Gonglei >> --- >> os-posix.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/os-posix.c b/os-posix.c >> index 9d5ae70..89831dc 100644 >> --- a/os-posix.c >> +++ b/os-posix.c >> @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename) >> return -1; >> } >> if (lockf(fd, F_TLOCK, 0) == -1) { >> + error_report("lock file '%s' failed: %s", filename, strerror(errno)); >> close(fd); >> return -1; >> } > > > I think I'll just revert this patch, because indeed, it makes > no sense to do this like that. > > Context: > > qemu_create_pidfile() is only created from main(), and there, > if that function returns failure, os_pidfile_error() function > is called, to, guess that, report error (which is done differently > whenever we're daemonizing or not). > > qemu_create_pidfile() function has several error returns, this > lockf() failure is one of them, there are others (another shown > in the patch context too). > Yes, agree. > So this patch makes whole thing inconsistent at least. > > If we need to show error message when we're daemonizing, it > looks like we should modify os_pidfile_error() routine to always > report error and only after that check for daemon mode. This way > all errors will be reported the same way. > In os_pidfile_error(), like below... if (write(fds[1], &status, 1) != 1) { perror("daemonize. Writing to pipe\n"); } will call its child process to report error: else if (status == 1) { fprintf(stderr, "Could not acquire pidfile\n"); exit(1); } I just want to make the error message more clear so that people can find the root reasons of errors as soon as possible. :) > But I really wonder if we actually need os_pidfile_error() in the > first place, why this can't be done in qemu_create_pidfile(). > > So, I'm reverting this change now, to be revisited very soon. > If you think that's better, I'm fine with it. Best regards, -Gonglei