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

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

On 11/09/2015 06:33 PM, Michael S. Tsirkin 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>
>> ---
>>   include/qemu/osdep.h |  1 +
>>   target-ppc/kvm.c     | 21 +++------------------
>>   util/oslib-posix.c   | 16 ++++++++++++++++
>>   util/oslib-win32.c   |  5 +++++
>>   4 files changed, 25 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index b568424..d4dde02 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -302,4 +302,5 @@ int qemu_read_password(char *buf, int buf_size);
>>    */
>>   pid_t qemu_fork(Error **errp);
>>
>> +size_t qemu_file_get_page_size(const char *mem_path);
>>   #endif
>> 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;
>>   }
>>
>>   static int find_max_supported_pagesize(Object *obj, void *opaque)
>> 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;
>> +    }
>> +
>> +    size = fd_getpagesize(fd);
>> +    qemu_close(fd);
>> +exit:
>> +    return size;
>> +}
>> +
>>   void os_mem_prealloc(int fd, char *area, size_t memory)
>>   {
>>       int ret;
>
> So this is opening the file for the sole purpose of
> doing the fstatfs on it. Seems strange, just do statfs instead.
> In fact, maybe we want statfs_getpagesize.

It is just to reuse the code of fd_getpagesize() which already has
the logic to check pagesize.

>
>> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>> index 09f9e98..a18aa87 100644
>> --- a/util/oslib-win32.c
>> +++ b/util/oslib-win32.c
>> @@ -462,6 +462,11 @@ size_t getpagesize(void)
>>       return system_info.dwPageSize;
>>   }
>>
>> +size_t qemu_file_get_page_size(const char *path)
>> +{
>> +    return getpagesize();
>> +}
>> +
>>   void os_mem_prealloc(int fd, char *area, size_t memory)
>>   {
>>       int i;
>
> And why is this needed on win32?

It is not actually used, just make osdep.h happy which has
qemu_file_get_page_size() declare.

BTW, i will drop this patch for now on.