From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40467) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZveIH-0005Hk-TJ for qemu-devel@nongnu.org; Sun, 08 Nov 2015 23:43:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZveIC-0002rT-Th for qemu-devel@nongnu.org; Sun, 08 Nov 2015 23:43:01 -0500 Received: from mga14.intel.com ([192.55.52.115]:36586) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZveIC-0002rP-Mf for qemu-devel@nongnu.org; Sun, 08 Nov 2015 23:42:56 -0500 References: <1446455617-129562-1-git-send-email-guangrong.xiao@linux.intel.com> <1446455617-129562-8-git-send-email-guangrong.xiao@linux.intel.com> <20151106153634.GU4180@thinpad.lan.raisama.net> From: Xiao Guangrong Message-ID: <564022D4.700@linux.intel.com> Date: Mon, 9 Nov 2015 12:36:36 +0800 MIME-Version: 1.0 In-Reply-To: <20151106153634.GU4180@thinpad.lan.raisama.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 07/35] util: introduce qemu_file_get_page_size() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: vsementsov@virtuozzo.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 11/06/2015 11:36 PM, Eduardo Habkost wrote: > On Mon, Nov 02, 2015 at 05:13:09PM +0800, Xiao Guangrong wrote: >> There are three places use the some logic to get the page size on >> the file path or file fd >> >> Windows did not support file hugepage, so it will return normal page >> for this case. And this interface has not been used on windows so far >> >> This patch introduces qemu_file_get_page_size() to unify the code >> >> Signed-off-by: Xiao Guangrong > [...] >> diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> index 914cef5..51437ff 100644 >> --- a/util/oslib-posix.c >> +++ b/util/oslib-posix.c >> @@ -340,7 +340,7 @@ static void sigbus_handler(int signal) >> siglongjmp(sigjump, 1); >> } >> >> -static size_t fd_getpagesize(int fd) >> +static size_t fd_getpagesize(int fd, Error **errp) >> { >> #ifdef CONFIG_LINUX >> struct statfs fs; >> @@ -351,7 +351,12 @@ static size_t fd_getpagesize(int fd) >> ret = fstatfs(fd, &fs); >> } while (ret != 0 && errno == EINTR); >> >> - if (ret == 0 && fs.f_type == HUGETLBFS_MAGIC) { >> + if (ret) { >> + error_setg_errno(errp, errno, "fstatfs is failed"); >> + return 0; >> + } >> + >> + if (fs.f_type == HUGETLBFS_MAGIC) { >> return fs.f_bsize; >> } > > You are changing os_mem_prealloc() behavior when fstatfs() fails, here. > Have you ensured there are no cases where fstatfs() fails but this code > is still expected to work? stat() is supported for all kinds of files, so failed stat() is caused by file is not exist or kernel internal error (e,g memory is not enough) or security check is not passed. Whichever we should not do any operation on the file if stat() failed. The origin code did not check it but it is worth being fixed i think. > > The change looks safe: gethugepagesize() seems to be always called in > the path that would make fd_getpagesize() be called from > os_mem_prealloc(), so allocation would abort much earlier if statfs() > failed. But I haven't confirmed that yet, and I wanted to be sure. > Yes, I am entirely agree with you. :)