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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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 <guangrong.xiao@xxxxxxxxxxxxxxx>
[...]
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. :)

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux