From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52176) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvsch-00074c-9h for qemu-devel@nongnu.org; Mon, 09 Nov 2015 15:01:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zvsce-0001B3-1s for qemu-devel@nongnu.org; Mon, 09 Nov 2015 15:01:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56994) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvscd-0001Ar-Ro for qemu-devel@nongnu.org; Mon, 09 Nov 2015 15:01:00 -0500 Date: Mon, 9 Nov 2015 22:00:52 +0200 From: "Michael S. Tsirkin" Message-ID: <20151109215812-mutt-send-email-mst@redhat.com> References: <1446184587-142784-1-git-send-email-guangrong.xiao@linux.intel.com> <1446184587-142784-8-git-send-email-guangrong.xiao@linux.intel.com> <20151030155454.GS4180@thinpad.lan.raisama.net> <56347754.6090904@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56347754.6090904@linux.intel.com> Subject: Re: [Qemu-devel] [PATCH v6 07/33] util: introduce qemu_file_get_page_size() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: Eduardo Habkost , kvm@vger.kernel.org, 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 Sat, Oct 31, 2015 at 04:09:56PM +0800, Xiao Guangrong wrote: > > > On 10/30/2015 11:54 PM, Eduardo Habkost wrote: > >On Fri, Oct 30, 2015 at 01:56:01PM +0800, Xiao Guangrong wrote: > >>There are three places use the some logic to get the page size on > >>the file path or file fd > >> > >>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..ad94c5a 100644 > >>--- a/util/oslib-posix.c > >>+++ b/util/oslib-posix.c > >>@@ -360,6 +360,22 @@ static size_t fd_getpagesize(int fd) > >> return getpagesize(); > >> } > >> > >>+size_t qemu_file_get_page_size(const char *path) > >>+{ > >>+ size_t size = 0; > >>+ int fd = qemu_open(path, O_RDONLY); > >>+ > >>+ if (fd < 0) { > >>+ fprintf(stderr, "Could not open %s.\n", path); > >>+ goto exit; > > > >Have you considered using a Error** argument here? > > > >>+ } > >>+ > >>+ size = fd_getpagesize(fd); > >>+ qemu_close(fd); > >>+exit: > >>+ return size; > >>+} > >>+ > >>diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > >>index ac70f08..c661f1c 100644 > >>--- a/target-ppc/kvm.c > >>+++ b/target-ppc/kvm.c > >>@@ -308,28 +308,13 @@ static void kvm_get_smmu_info(PowerPCCPU *cpu, struct kvm_ppc_smmu_info *info) > >> > >> static long gethugepagesize(const char *mem_path) > >> { > >>- struct statfs fs; > >>- int ret; > >>- > >>- do { > >>- ret = statfs(mem_path, &fs); > >>- } while (ret != 0 && errno == EINTR); > >>+ long size = qemu_file_get_page_size(mem_path); > >> > >>- if (ret != 0) { > >>- fprintf(stderr, "Couldn't statfs() memory path: %s\n", > >>- strerror(errno)); > >>+ if (!size) { > >> exit(1); > >> } > >> > >>-#define HUGETLBFS_MAGIC 0x958458f6 > >>- > >>- if (fs.f_type != HUGETLBFS_MAGIC) { > >>- /* Explicit mempath, but it's ordinary pages */ > >>- return getpagesize(); > >>- } > >>- > >>- /* It's hugepage, return the huge page size */ > >>- return fs.f_bsize; > >>+ return size; > >> } > > > >Why are you changing target-ppc/kvm.c:gethugepagesize() to use the new > >funtion, but not the copy at exec.c? To make it simpler, we could > >eliminate both gethugepagesize() functions completely and replace them > >with qemu_file_get_page_size() calls (maybe as part of this patch, maybe > >in a separate patch, I'm not sure). > > > > The gethugepagesize() in exec.c will be eliminated in later patch :). That's why it's not a good idea to split patchset like this, where patch 1 adds a new function, patch 2 uses it. It's better if user is in the same patchset. An exception if when a completely separate group of people should review the function and the usage, e.g. some logic in memory core versus caller in acpi. > And the gethugepagesize() in ppc platform has error handling logic > and has multiple caller. It's not so bad to keep it. > -- MST