[Qemu-devel] [PATCH v2] hostmem-file: reject invalid pmem file sizes

[Qemu-devel] [PATCH v2] hostmem-file: reject invalid pmem file sizes

[Stefan Hajnoczi]

Guests started with NVDIMMs larger than the underlying host file produce
confusing errors inside the guest.  This happens because the guest
accesses pages beyond the end of the file.

Check the pmem file size on startup and print a clear error message if
the size is invalid.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1669053
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Cc: Zhang Yi <yi.z.zhang@linux.intel.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
v2:
 * Propagate qemu_get_pmem_size() errors [Igor]

 include/qemu/osdep.h    | 13 ++++++++++
 backends/hostmem-file.c | 22 +++++++++++++++++
 util/oslib-posix.c      | 53 +++++++++++++++++++++++++++++++++++++++++
 util/oslib-win32.c      |  5 ++++
 4 files changed, 93 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 840af09cb0..303d315c5d 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -570,6 +570,19 @@ void qemu_set_tty_echo(int fd, bool echo);
 void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
                      Error **errp);
 
+/**
+ * qemu_get_pmem_size:
+ * @filename: path to a pmem file
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Determine the size of a persistent memory file.  Besides supporting files on
+ * DAX file systems, this function also supports Linux devdax character
+ * devices.
+ *
+ * Returns: the size or 0 on failure
+ */
+uint64_t qemu_get_pmem_size(const char *filename, Error **errp);
+
 /**
  * qemu_get_pid_name:
  * @pid: pid of a process
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index ba601ce940..d62689179b 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -46,6 +46,28 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     gchar *name;
 #endif
 
+    /*
+     * Verify pmem file size since starting a guest with an incorrect size
+     * leads to confusing failures inside the guest.
+     */
+    if (fb->is_pmem && fb->mem_path) {
+        Error *local_err = NULL;
+        uint64_t size;
+
+        size = qemu_get_pmem_size(fb->mem_path, &local_err);
+        if (!size) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        if (backend->size > size) {
+            error_setg(errp, "size property %" PRIu64 " is larger than "
+                       "pmem file \"%s\" size %" PRIu64, backend->size,
+                       fb->mem_path, size);
+            return;
+        }
+    }
+
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
         return;
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 37c5854b9c..10d90d1783 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -500,6 +500,59 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
     }
 }
 
+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
+{
+    struct stat st;
+
+    if (stat(filename, &st) < 0) {
+        error_setg(errp, "unable to stat pmem file \"%s\"", filename);
+        return 0;
+    }
+
+#if defined(__linux__)
+    /* Special handling for devdax character devices */
+    if (S_ISCHR(st.st_mode)) {
+        char *subsystem_path = NULL;
+        char *subsystem = NULL;
+        char *size_path = NULL;
+        char *size_str = NULL;
+        uint64_t ret = 0;
+
+        subsystem_path = g_strdup_printf("/sys/dev/char/%d:%d/subsystem",
+                                         major(st.st_rdev), minor(st.st_rdev));
+        subsystem = g_file_read_link(subsystem_path, NULL);
+        if (!subsystem) {
+            error_setg(errp, "unable to read subsystem for pmem file \"%s\"",
+                       filename);
+            goto devdax_err;
+        }
+
+        if (!g_str_has_suffix(subsystem, "/dax")) {
+            error_setg(errp, "pmem file \"%s\" is not a dax device", filename);
+            goto devdax_err;
+        }
+
+        size_path = g_strdup_printf("/sys/dev/char/%d:%d/size",
+                                    major(st.st_rdev), minor(st.st_rdev));
+        if (!g_file_get_contents(size_path, &size_str, NULL, NULL)) {
+            error_setg(errp, "unable to read size for pmem file \"%s\"",
+                       size_path);
+            goto devdax_err;
+        }
+
+        ret = g_ascii_strtoull(size_str, NULL, 0);
+
+devdax_err:
+        g_free(size_str);
+        g_free(size_path);
+        g_free(subsystem);
+        g_free(subsystem_path);
+        return ret;
+    }
+#endif /* defined(__linux__) */
+
+    return st.st_size;
+}
 
 char *qemu_get_pid_name(pid_t pid)
 {
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index b4c17f5dfa..bd633afab6 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -560,6 +560,11 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
     }
 }
 
+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
+{
+    error_setg(errp, "pmem support not available");
+    return 0;
+}
 
 char *qemu_get_pid_name(pid_t pid)
 {
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2] hostmem-file: reject invalid pmem file sizes

[Wei Yang]

On Wed, Feb 13, 2019 at 03:14:01PM +0800, Stefan Hajnoczi wrote:
>Guests started with NVDIMMs larger than the underlying host file produce
>confusing errors inside the guest.  This happens because the guest
>accesses pages beyond the end of the file.
>
>Check the pmem file size on startup and print a clear error message if
>the size is invalid.
>
>Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1669053
>Cc: Haozhong Zhang <haozhong.zhang@intel.com>
>Cc: Zhang Yi <yi.z.zhang@linux.intel.com>
>Cc: Eduardo Habkost <ehabkost@redhat.com>
>Cc: Igor Mammedov <imammedo@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
>v2:
> * Propagate qemu_get_pmem_size() errors [Igor]
>
> include/qemu/osdep.h    | 13 ++++++++++
> backends/hostmem-file.c | 22 +++++++++++++++++
> util/oslib-posix.c      | 53 +++++++++++++++++++++++++++++++++++++++++
> util/oslib-win32.c      |  5 ++++
> 4 files changed, 93 insertions(+)
>
>diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>index 840af09cb0..303d315c5d 100644
>--- a/include/qemu/osdep.h
>+++ b/include/qemu/osdep.h
>@@ -570,6 +570,19 @@ void qemu_set_tty_echo(int fd, bool echo);
> void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
>                      Error **errp);
> 
>+/**
>+ * qemu_get_pmem_size:
>+ * @filename: path to a pmem file
>+ * @errp: pointer to a NULL-initialized error object
>+ *
>+ * Determine the size of a persistent memory file.  Besides supporting files on
>+ * DAX file systems, this function also supports Linux devdax character
>+ * devices.
>+ *
>+ * Returns: the size or 0 on failure
>+
> /**
>  * qemu_get_pid_name:
>  * @pid: pid of a process
>diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>index ba601ce940..d62689179b 100644
>--- a/backends/hostmem-file.c
>+++ b/backends/hostmem-file.c
>@@ -46,6 +46,28 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>     gchar *name;
> #endif
> 
>+    /*
>+     * Verify pmem file size since starting a guest with an incorrect size
>+     * leads to confusing failures inside the guest.
>+     */
>+    if (fb->is_pmem && fb->mem_path) {
>+        Error *local_err = NULL;
>+        uint64_t size;
>+
>+        size = qemu_get_pmem_size(fb->mem_path, &local_err);
>+        if (!size) {
>+            error_propagate(errp, local_err);
>+            return;
>+        }
>+
>+        if (backend->size > size) {
>+            error_setg(errp, "size property %" PRIu64 " is larger than "
>+                       "pmem file \"%s\" size %" PRIu64, backend->size,
>+                       fb->mem_path, size);
>+            return;
>+        }
>+    }
>+
>     if (!backend->size) {
>         error_setg(errp, "can't create backend with size 0");
>         return;

Would it be reasonable to move the above check after checking size and
mem_path?

>diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>index 37c5854b9c..10d90d1783 100644
>--- a/util/oslib-posix.c
>+++ b/util/oslib-posix.c
>@@ -500,6 +500,59 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>     }
> }
> 
>+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
>+{
>+    struct stat st;
>+
>+    if (stat(filename, &st) < 0) {
>+        error_setg(errp, "unable to stat pmem file \"%s\"", filename);
>+        return 0;
>+    }
>+
>+#if defined(__linux__)
>+    /* Special handling for devdax character devices */
>+    if (S_ISCHR(st.st_mode)) {
>+        char *subsystem_path = NULL;
>+        char *subsystem = NULL;
>+        char *size_path = NULL;
>+        char *size_str = NULL;
>+        uint64_t ret = 0;
>+
>+        subsystem_path = g_strdup_printf("/sys/dev/char/%d:%d/subsystem",
>+                                         major(st.st_rdev), minor(st.st_rdev));
>+        subsystem = g_file_read_link(subsystem_path, NULL);
>+        if (!subsystem) {
>+            error_setg(errp, "unable to read subsystem for pmem file \"%s\"",
>+                       filename);
>+            goto devdax_err;
>+        }
>+
>+        if (!g_str_has_suffix(subsystem, "/dax")) {
>+            error_setg(errp, "pmem file \"%s\" is not a dax device", filename);
>+            goto devdax_err;
>+        }
>+
>+        size_path = g_strdup_printf("/sys/dev/char/%d:%d/size",
>+                                    major(st.st_rdev), minor(st.st_rdev));
>+        if (!g_file_get_contents(size_path, &size_str, NULL, NULL)) {
>+            error_setg(errp, "unable to read size for pmem file \"%s\"",
>+                       size_path);
>+            goto devdax_err;
>+        }
>+
>+        ret = g_ascii_strtoull(size_str, NULL, 0);
>+
>+devdax_err:
>+        g_free(size_str);
>+        g_free(size_path);
>+        g_free(subsystem);
>+        g_free(subsystem_path);
>+        return ret;
>+    }
>+#endif /* defined(__linux__) */
>+
>+    return st.st_size;
>+}
> 
> char *qemu_get_pid_name(pid_t pid)
> {
>diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>index b4c17f5dfa..bd633afab6 100644
>--- a/util/oslib-win32.c
>+++ b/util/oslib-win32.c
>@@ -560,6 +560,11 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>     }
> }
> 
>+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
>+{
>+    error_setg(errp, "pmem support not available");
>+    return 0;
>+}
> 
> char *qemu_get_pid_name(pid_t pid)
> {
>-- 
>2.20.1
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2] hostmem-file: reject invalid pmem file sizes

[Wei Yang]

On Wed, Feb 13, 2019 at 03:14:01PM +0800, Stefan Hajnoczi wrote:
>Guests started with NVDIMMs larger than the underlying host file produce
>confusing errors inside the guest.  This happens because the guest
>accesses pages beyond the end of the file.
>
>Check the pmem file size on startup and print a clear error message if
>the size is invalid.
>
>Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1669053
>Cc: Haozhong Zhang <haozhong.zhang@intel.com>
>Cc: Zhang Yi <yi.z.zhang@linux.intel.com>
>Cc: Eduardo Habkost <ehabkost@redhat.com>
>Cc: Igor Mammedov <imammedo@redhat.com>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
>v2:
> * Propagate qemu_get_pmem_size() errors [Igor]
>
> include/qemu/osdep.h    | 13 ++++++++++
> backends/hostmem-file.c | 22 +++++++++++++++++
> util/oslib-posix.c      | 53 +++++++++++++++++++++++++++++++++++++++++
> util/oslib-win32.c      |  5 ++++
> 4 files changed, 93 insertions(+)
>
>diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>index 840af09cb0..303d315c5d 100644
>--- a/include/qemu/osdep.h
>+++ b/include/qemu/osdep.h
>@@ -570,6 +570,19 @@ void qemu_set_tty_echo(int fd, bool echo);
> void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
>                      Error **errp);
> 
>+/**
>+ * qemu_get_pmem_size:
>+ * @filename: path to a pmem file
>+ * @errp: pointer to a NULL-initialized error object
>+ *
>+ * Determine the size of a persistent memory file.  Besides supporting files on
>+ * DAX file systems, this function also supports Linux devdax character
>+ * devices.
>+ *
>+ * Returns: the size or 0 on failure
>+ */
>+uint64_t qemu_get_pmem_size(const char *filename, Error **errp);
>+
> /**
>  * qemu_get_pid_name:
>  * @pid: pid of a process
>diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>index ba601ce940..d62689179b 100644
>--- a/backends/hostmem-file.c
>+++ b/backends/hostmem-file.c
>@@ -46,6 +46,28 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>     gchar *name;
> #endif
> 
>+    /*
>+     * Verify pmem file size since starting a guest with an incorrect size
>+     * leads to confusing failures inside the guest.
>+     */
>+    if (fb->is_pmem && fb->mem_path) {
>+        Error *local_err = NULL;
>+        uint64_t size;
>+
>+        size = qemu_get_pmem_size(fb->mem_path, &local_err);
>+        if (!size) {
>+            error_propagate(errp, local_err);
>+            return;
>+        }
>+
>+        if (backend->size > size) {
>+            error_setg(errp, "size property %" PRIu64 " is larger than "
>+                       "pmem file \"%s\" size %" PRIu64, backend->size,
>+                       fb->mem_path, size);
>+            return;
>+        }
>+    }
>+

-mem-path is only supported on POSIX host.

So how about put this in the last code block before doing the real memory
region init?

BTW, I suggest to cleanup file_backend_memory_alloc() to make POSIX/non-POSIX
situation clear to understand.

>     if (!backend->size) {
>         error_setg(errp, "can't create backend with size 0");
>         return;
>diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>index 37c5854b9c..10d90d1783 100644
>--- a/util/oslib-posix.c
>+++ b/util/oslib-posix.c
>@@ -500,6 +500,59 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>     }
> }
> 
>+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
>+{
>+    struct stat st;
>+
>+    if (stat(filename, &st) < 0) {
>+        error_setg(errp, "unable to stat pmem file \"%s\"", filename);
>+        return 0;
>+    }
>+
>+#if defined(__linux__)
>+    /* Special handling for devdax character devices */
>+    if (S_ISCHR(st.st_mode)) {
>+        char *subsystem_path = NULL;
>+        char *subsystem = NULL;
>+        char *size_path = NULL;
>+        char *size_str = NULL;
>+        uint64_t ret = 0;
>+
>+        subsystem_path = g_strdup_printf("/sys/dev/char/%d:%d/subsystem",
>+                                         major(st.st_rdev), minor(st.st_rdev));
>+        subsystem = g_file_read_link(subsystem_path, NULL);
>+        if (!subsystem) {
>+            error_setg(errp, "unable to read subsystem for pmem file \"%s\"",
>+                       filename);
>+            goto devdax_err;
>+        }
>+
>+        if (!g_str_has_suffix(subsystem, "/dax")) {
>+            error_setg(errp, "pmem file \"%s\" is not a dax device", filename);
>+            goto devdax_err;
>+        }
>+
>+        size_path = g_strdup_printf("/sys/dev/char/%d:%d/size",
>+                                    major(st.st_rdev), minor(st.st_rdev));
>+        if (!g_file_get_contents(size_path, &size_str, NULL, NULL)) {
>+            error_setg(errp, "unable to read size for pmem file \"%s\"",
>+                       size_path);
>+            goto devdax_err;
>+        }
>+
>+        ret = g_ascii_strtoull(size_str, NULL, 0);
>+
>+devdax_err:
>+        g_free(size_str);
>+        g_free(size_path);
>+        g_free(subsystem);
>+        g_free(subsystem_path);
>+        return ret;
>+    }
>+#endif /* defined(__linux__) */
>+
>+    return st.st_size;
>+}
> 
> char *qemu_get_pid_name(pid_t pid)
> {
>diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>index b4c17f5dfa..bd633afab6 100644
>--- a/util/oslib-win32.c
>+++ b/util/oslib-win32.c
>@@ -560,6 +560,11 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>     }
> }
> 
>+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
>+{
>+    error_setg(errp, "pmem support not available");
>+    return 0;
>+}
> 
> char *qemu_get_pid_name(pid_t pid)
> {
>-- 
>2.20.1
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2] hostmem-file: reject invalid pmem file sizes

[Igor Mammedov]

On Wed, 13 Feb 2019 16:53:54 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Wed, Feb 13, 2019 at 03:14:01PM +0800, Stefan Hajnoczi wrote:
> >Guests started with NVDIMMs larger than the underlying host file produce
> >confusing errors inside the guest.  This happens because the guest
> >accesses pages beyond the end of the file.
> >
> >Check the pmem file size on startup and print a clear error message if
> >the size is invalid.
> >
> >Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1669053
> >Cc: Haozhong Zhang <haozhong.zhang@intel.com>
> >Cc: Zhang Yi <yi.z.zhang@linux.intel.com>
> >Cc: Eduardo Habkost <ehabkost@redhat.com>
> >Cc: Igor Mammedov <imammedo@redhat.com>
> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >---
> >v2:
> > * Propagate qemu_get_pmem_size() errors [Igor]
> >
> > include/qemu/osdep.h    | 13 ++++++++++
> > backends/hostmem-file.c | 22 +++++++++++++++++
> > util/oslib-posix.c      | 53 +++++++++++++++++++++++++++++++++++++++++
> > util/oslib-win32.c      |  5 ++++
> > 4 files changed, 93 insertions(+)
> >
> >diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> >index 840af09cb0..303d315c5d 100644
> >--- a/include/qemu/osdep.h
> >+++ b/include/qemu/osdep.h
> >@@ -570,6 +570,19 @@ void qemu_set_tty_echo(int fd, bool echo);
> > void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
> >                      Error **errp);
> > 
> >+/**
> >+ * qemu_get_pmem_size:
> >+ * @filename: path to a pmem file
> >+ * @errp: pointer to a NULL-initialized error object
> >+ *
> >+ * Determine the size of a persistent memory file.  Besides supporting files on
> >+ * DAX file systems, this function also supports Linux devdax character
> >+ * devices.
> >+ *
> >+ * Returns: the size or 0 on failure
> >+ */
> >+uint64_t qemu_get_pmem_size(const char *filename, Error **errp);
> >+
> > /**
> >  * qemu_get_pid_name:
> >  * @pid: pid of a process
> >diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> >index ba601ce940..d62689179b 100644
> >--- a/backends/hostmem-file.c
> >+++ b/backends/hostmem-file.c
> >@@ -46,6 +46,28 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >     gchar *name;
> > #endif
> > 
> >+    /*
> >+     * Verify pmem file size since starting a guest with an incorrect size
> >+     * leads to confusing failures inside the guest.
> >+     */
> >+    if (fb->is_pmem && fb->mem_path) {
> >+        Error *local_err = NULL;
> >+        uint64_t size;
> >+
> >+        size = qemu_get_pmem_size(fb->mem_path, &local_err);
> >+        if (!size) {
> >+            error_propagate(errp, local_err);
> >+            return;
> >+        }
> >+
> >+        if (backend->size > size) {
> >+            error_setg(errp, "size property %" PRIu64 " is larger than "
> >+                       "pmem file \"%s\" size %" PRIu64, backend->size,
> >+                       fb->mem_path, size);
> >+            return;
> >+        }
> >+    }
> >+  
> 
> -mem-path is only supported on POSIX host.
> 
> So how about put this in the last code block before doing the real memory
> region init?
Indeed, evaluating file would be better done only if we didn't fail size/mempath checks first.

 
> BTW, I suggest to cleanup file_backend_memory_alloc() to make POSIX/non-POSIX
> situation clear to understand.
that's a separate patch, I'll reply with it here so Sefan could pick it (due to conflicts the change would create)

> 
> >     if (!backend->size) {
> >         error_setg(errp, "can't create backend with size 0");
> >         return;
> >diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> >index 37c5854b9c..10d90d1783 100644
> >--- a/util/oslib-posix.c
> >+++ b/util/oslib-posix.c
> >@@ -500,6 +500,59 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
> >     }
> > }
> > 
> >+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
> >+{
> >+    struct stat st;
> >+
> >+    if (stat(filename, &st) < 0) {
> >+        error_setg(errp, "unable to stat pmem file \"%s\"", filename);
> >+        return 0;
> >+    }
> >+
> >+#if defined(__linux__)
> >+    /* Special handling for devdax character devices */
> >+    if (S_ISCHR(st.st_mode)) {
> >+        char *subsystem_path = NULL;
> >+        char *subsystem = NULL;
> >+        char *size_path = NULL;
> >+        char *size_str = NULL;
> >+        uint64_t ret = 0;
> >+
> >+        subsystem_path = g_strdup_printf("/sys/dev/char/%d:%d/subsystem",
> >+                                         major(st.st_rdev), minor(st.st_rdev));
> >+        subsystem = g_file_read_link(subsystem_path, NULL);
> >+        if (!subsystem) {
> >+            error_setg(errp, "unable to read subsystem for pmem file \"%s\"",
> >+                       filename);
> >+            goto devdax_err;
> >+        }
> >+
> >+        if (!g_str_has_suffix(subsystem, "/dax")) {
> >+            error_setg(errp, "pmem file \"%s\" is not a dax device", filename);
> >+            goto devdax_err;
> >+        }
> >+
> >+        size_path = g_strdup_printf("/sys/dev/char/%d:%d/size",
> >+                                    major(st.st_rdev), minor(st.st_rdev));
> >+        if (!g_file_get_contents(size_path, &size_str, NULL, NULL)) {
> >+            error_setg(errp, "unable to read size for pmem file \"%s\"",
> >+                       size_path);
> >+            goto devdax_err;
> >+        }
> >+
> >+        ret = g_ascii_strtoull(size_str, NULL, 0);
> >+
> >+devdax_err:
> >+        g_free(size_str);
> >+        g_free(size_path);
> >+        g_free(subsystem);
> >+        g_free(subsystem_path);
> >+        return ret;
> >+    }
> >+#endif /* defined(__linux__) */
> >+
> >+    return st.st_size;
> >+}
> > 
> > char *qemu_get_pid_name(pid_t pid)
> > {
> >diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> >index b4c17f5dfa..bd633afab6 100644
> >--- a/util/oslib-win32.c
> >+++ b/util/oslib-win32.c
> >@@ -560,6 +560,11 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
> >     }
> > }
> > 
> >+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
> >+{
> >+    error_setg(errp, "pmem support not available");
> >+    return 0;
> >+}
> > 
> > char *qemu_get_pid_name(pid_t pid)
> > {
> >-- 
> >2.20.1
> >  
> 

[Qemu-devel] [PATCH] hostmem: simplify ifdef-s in file_backend_memory_alloc()

[Igor Mammedov]

cleanup file_backend_memory_alloc() by using one CONFIG_POSIX ifdef
instead of several ones within the function to make it simpler to follow.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Suggested-by: Wei Yang <richardw.yang@linux.intel.com>
---
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Cc: Zhang Yi <yi.z.zhang@linux.intel.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
CC: Wei Yang <richardw.yang@linux.intel.com>
---
 backends/hostmem-file.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index ba601ce940..ce54788048 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -41,10 +41,12 @@ struct HostMemoryBackendFile {
 static void
 file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
+#ifndef CONFIG_POSIX
+    error_setg(errp, "backend '%s' not supported on this host",
+               object_get_typename(OBJECT(backend)));
+#else
     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
-#ifdef CONFIG_POSIX
     gchar *name;
-#endif
 
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
@@ -54,9 +56,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         error_setg(errp, "mem-path property not set");
         return;
     }
-#ifndef CONFIG_POSIX
-    error_setg(errp, "-mem-path not supported on this host");
-#else
     backend->force_prealloc = mem_prealloc;
     name = host_memory_backend_get_name(backend);
     memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
-- 
2.18.1

Re: [Qemu-devel] [PATCH] hostmem: simplify ifdef-s in file_backend_memory_alloc()

[Wei Yang]

On Wed, Feb 13, 2019 at 07:38:58AM -0500, Igor Mammedov wrote:
>cleanup file_backend_memory_alloc() by using one CONFIG_POSIX ifdef
>instead of several ones within the function to make it simpler to follow.
>
>Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>Suggested-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>

>---
>Cc: Haozhong Zhang <haozhong.zhang@intel.com>
>Cc: Zhang Yi <yi.z.zhang@linux.intel.com>
>Cc: Eduardo Habkost <ehabkost@redhat.com>
>Cc: Stefan Hajnoczi <stefanha@redhat.com>
>CC: Wei Yang <richardw.yang@linux.intel.com>
>---
> backends/hostmem-file.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
>diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>index ba601ce940..ce54788048 100644
>--- a/backends/hostmem-file.c
>+++ b/backends/hostmem-file.c
>@@ -41,10 +41,12 @@ struct HostMemoryBackendFile {
> static void
> file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> {
>+#ifndef CONFIG_POSIX
>+    error_setg(errp, "backend '%s' not supported on this host",
>+               object_get_typename(OBJECT(backend)));
>+#else
>     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>-#ifdef CONFIG_POSIX
>     gchar *name;
>-#endif
> 
>     if (!backend->size) {
>         error_setg(errp, "can't create backend with size 0");
>@@ -54,9 +56,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>         error_setg(errp, "mem-path property not set");
>         return;
>     }
>-#ifndef CONFIG_POSIX
>-    error_setg(errp, "-mem-path not supported on this host");
>-#else
>     backend->force_prealloc = mem_prealloc;
>     name = host_memory_backend_get_name(backend);
>     memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>-- 
>2.18.1
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2] hostmem-file: reject invalid pmem file sizes

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On Wed, Feb 13, 2019 at 04:13:18PM +0800, Wei Yang wrote:
> On Wed, Feb 13, 2019 at 03:14:01PM +0800, Stefan Hajnoczi wrote:
> >diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> >index ba601ce940..d62689179b 100644
> >--- a/backends/hostmem-file.c
> >+++ b/backends/hostmem-file.c
> >@@ -46,6 +46,28 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >     gchar *name;
> > #endif
> > 
> >+    /*
> >+     * Verify pmem file size since starting a guest with an incorrect size
> >+     * leads to confusing failures inside the guest.
> >+     */
> >+    if (fb->is_pmem && fb->mem_path) {
> >+        Error *local_err = NULL;
> >+        uint64_t size;
> >+
> >+        size = qemu_get_pmem_size(fb->mem_path, &local_err);
> >+        if (!size) {
> >+            error_propagate(errp, local_err);
> >+            return;
> >+        }
> >+
> >+        if (backend->size > size) {
> >+            error_setg(errp, "size property %" PRIu64 " is larger than "
> >+                       "pmem file \"%s\" size %" PRIu64, backend->size,
> >+                       fb->mem_path, size);
> >+            return;
> >+        }
> >+    }
> >+
> >     if (!backend->size) {
> >         error_setg(errp, "can't create backend with size 0");
> >         return;
> 
> Would it be reasonable to move the above check after checking size and
> mem_path?

Thanks, will fix in v3.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]