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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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 <guangrong.xiao@linux.intel.com>
> [...]
>>>> 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!