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

[Xiao Guangrong]

On 10/31/2015 10:11 PM, Eduardo Habkost wrote:
> 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/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 true. I was just expecting to see the change that eliminates
> gethugepagesize() in exec.c here instead of patch 08/33. Simple cleanups
> that just eliminate code duplication without change in behavior are
> easier to review and more likely to be included before the rest of the
> series.

Completely agree, will move the replacement from 08/33 to here in
the next version.

> > And the gethugepagesize() in ppc platform has error handling logic
> > and has multiple caller. It's not so bad to keep it.
>
> Well, in case there's still room for cleanup in the ppc code, it may be
> done later. No problem.

Thank you, Eduardo!