Re: [Qemu-devel] [PATCH v7 07/35] util: introduce qemu_file_get_page_size()

[Xiao Guangrong]

On 11/06/2015 11:36 PM, Eduardo Habkost wrote:
> On Mon, Nov 02, 2015 at 05:13:09PM +0800, Xiao Guangrong wrote:
> > There are three places use the some logic to get the page size on
> > the file path or file fd
> >
> > Windows did not support file hugepage, so it will return normal page
> > for this case. And this interface has not been used on windows so far
> >
> > 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..51437ff 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -340,7 +340,7 @@ static void sigbus_handler(int signal)
> >       siglongjmp(sigjump, 1);
> >   }
> >
> > -static size_t fd_getpagesize(int fd)
> > +static size_t fd_getpagesize(int fd, Error **errp)
> >   {
> >   #ifdef CONFIG_LINUX
> >       struct statfs fs;
> > @@ -351,7 +351,12 @@ static size_t fd_getpagesize(int fd)
> >               ret = fstatfs(fd, &fs);
> >           } while (ret != 0 && errno == EINTR);
> >
> > -        if (ret == 0 && fs.f_type == HUGETLBFS_MAGIC) {
> > +        if (ret) {
> > +            error_setg_errno(errp, errno, "fstatfs is failed");
> > +            return 0;
> > +        }
> > +
> > +        if (fs.f_type == HUGETLBFS_MAGIC) {
> >               return fs.f_bsize;
> >           }
>
> You are changing os_mem_prealloc() behavior when fstatfs() fails, here.
> Have you ensured there are no cases where fstatfs() fails but this code
> is still expected to work?

stat() is supported for all kinds of files, so failed stat() is caused by
file is not exist or kernel internal error (e,g memory is not enough) or
security check is not passed. Whichever we should not do any operation on
the file if stat() failed. The origin code did not check it but it is worth
being fixed i think.

> The change looks safe: gethugepagesize() seems to be always called in
> the path that would make fd_getpagesize() be called from
> os_mem_prealloc(), so allocation would abort much earlier if statfs()
> failed. But I haven't confirmed that yet, and I wanted to be sure.

Yes, I am entirely agree with you. :)