Re: [Qemu-devel] [PATCH v6 07/33] util: introduce qemu_file_get_page_size()

[Michael S. Tsirkin]

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 <address@hidden>
> >[...]
> >>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