[Qemu-devel] [PATCH v2 0/3] Improve truncation behavior of memory-backend-file

[Qemu-devel] [PATCH v2 0/3] Improve truncation behavior of memory-backend-file

[Haozhong Zhang]

For a mmeory backend file, e.g.
    -object memory-backend-file,mem-path=foo,size=SZ,...
the size of the backend file 'foo' is specified by the 'size'
option. If the specified size 'SZ' does not match the actual size of
file 'foo', QEMU will truncate the backend file 'foo'. In certain
usage scenarios (e.g. vNVDIMM), the truncation may corrupt the
existing data in the file.

Patch 1 in this series avoids such data corruption by disabling
truncating non-empty backend files. If a non-existing file, an empty
file or a directory is specified by 'mem-path' option, QEMU will
truncate the backend file to the size specified by 'size' option.

Patch 2 adds an additional check to avoid creating a memory backend
that can not be hold in the backend file. For a non-empty backend
file, if its size is smaller than 'size' option, QEMU will report
error.

Patch 3 makes the option 'size' optional. It's to avoid the misusing
of 'size' option. If the user is uncertain about the backend file
size, they can skip the 'size' option and let QEMU use the actual file
size.  If a non-existing file, an empty file or a directory is
specified by 'mem-path' option, the 'size' option is still needed.

Changes since v1:
 * Fix errors in v1 patches.
 * Split truncation skip and size check into separate patches.
 * Do not error out for backend file whose size is unknown.
 * Only error out when file size is smaller than 'size' option.
 * Change the error handling path of file_backend_memory_alloc().
 * Do not duplicate the setting of block->used_length/max_length in
   file_ram_alloc().


Haozhong Zhang (3):
  exec.c: do not truncate non-empty memory backend file
  exec.c: check memory backend file size with 'size' option
  hostmem-file: make option 'size' optional

 backends/hostmem-file.c | 28 ++++++++++++++++++-------
 exec.c                  | 56 +++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 65 insertions(+), 19 deletions(-)

-- 
2.10.1

[Qemu-devel] [PATCH v2 1/3] exec.c: do not truncate non-empty memory backend file

[Haozhong Zhang]

For '-object memory-backend-file,mem-path=foo,size=xyz', if the size of
file 'foo' does not match the given size 'xyz', the current QEMU will
truncate the file to the given size, which may corrupt the existing data
in that file. To avoid such data corruption, this patch disables
truncating non-empty backend files.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 exec.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 587b489..a2b371a 100644
--- a/exec.c
+++ b/exec.c
@@ -1224,6 +1224,15 @@ void qemu_mutex_unlock_ramlist(void)
 }
 
 #ifdef __linux__
+static int64_t get_file_size(int fd)
+{
+    int64_t size = lseek(fd, 0, SEEK_END);
+    if (size < 0) {
+        return -errno;
+    }
+    return size;
+}
+
 static void *file_ram_alloc(RAMBlock *block,
                             ram_addr_t memory,
                             const char *path,
@@ -1235,6 +1244,7 @@ static void *file_ram_alloc(RAMBlock *block,
     char *c;
     void *area = MAP_FAILED;
     int fd = -1;
+    int64_t file_size;
 
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
         error_setg(errp,
@@ -1297,6 +1307,8 @@ static void *file_ram_alloc(RAMBlock *block,
     }
 #endif
 
+    file_size = get_file_size(fd);
+
     if (memory < block->page_size) {
         error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
                    "or larger than page size 0x%zx",
@@ -1311,8 +1323,16 @@ static void *file_ram_alloc(RAMBlock *block,
      * hosts, so don't bother bailing out on errors.
      * If anything goes wrong with it under other filesystems,
      * mmap will fail.
+     *
+     * Do not truncate the non-empty backend file to avoid corrupting
+     * the existing data in the file. Disabling shrinking is not
+     * enough. For example, the current vNVDIMM implementation stores
+     * the guest NVDIMM labels at the end of the backend file. If the
+     * backend file is later extended, QEMU will not be able to find
+     * those labels. Therefore, extending the non-empty backend file
+     * is disabled as well.
      */
-    if (ftruncate(fd, memory)) {
+    if (!file_size && ftruncate(fd, memory)) {
         perror("ftruncate");
     }
 
-- 
2.10.1

[Qemu-devel] [PATCH v2 2/3] exec.c: check memory backend file size with 'size' option

[Haozhong Zhang]

If the memory backend file is not large enough to hold the required 'size',
Qemu will report error and exit.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 exec.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/exec.c b/exec.c
index a2b371a..264a25f 100644
--- a/exec.c
+++ b/exec.c
@@ -1316,6 +1316,13 @@ static void *file_ram_alloc(RAMBlock *block,
         goto error;
     }
 
+    if (file_size > 0 && file_size < memory) {
+        error_setg(errp, "backing store %s size %"PRId64
+                   " does not match 'size' option %"PRIu64,
+                   path, file_size, memory);
+        goto error;
+    }
+
     memory = ROUND_UP(memory, block->page_size);
 
     /*
-- 
2.10.1

[Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optional

[Haozhong Zhang]

If 'size' option is not given, Qemu will use the file size of 'mem-path'
instead. If an empty file, a non-existing file or a directory is specified
by option 'mem-path', a non-zero option 'size' is still needed.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 backends/hostmem-file.c | 28 ++++++++++++++++++++--------
 exec.c                  | 33 ++++++++++++++++++++-------------
 2 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 42efb2f..6ee4352 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -39,17 +39,14 @@ static void
 file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
+    Error *local_err = NULL;
 
-    if (!backend->size) {
-        error_setg(errp, "can't create backend with size 0");
-        return;
-    }
     if (!fb->mem_path) {
-        error_setg(errp, "mem-path property not set");
-        return;
+        error_setg(&local_err, "mem-path property not set");
+        goto out;
     }
 #ifndef CONFIG_LINUX
-    error_setg(errp, "-mem-path not supported on this host");
+    error_setg(&local_err, "-mem-path not supported on this host");
 #else
     if (!memory_region_size(&backend->mr)) {
         gchar *path;
@@ -58,10 +55,25 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
                                  path,
                                  backend->size, fb->share,
-                                 fb->mem_path, errp);
+                                 fb->mem_path, &local_err);
         g_free(path);
+
+        if (local_err) {
+            goto out;
+        }
+
+        if (!backend->size) {
+            backend->size = memory_region_size(&backend->mr);
+        }
     }
 #endif
+
+    if (!backend->size) {
+        error_setg(&local_err, "can't create backend with size 0");
+    }
+
+ out:
+    error_propagate(errp, local_err);
 }
 
 static char *get_mem_path(Object *o, Error **errp)
diff --git a/exec.c b/exec.c
index 264a25f..89065bd 100644
--- a/exec.c
+++ b/exec.c
@@ -1234,7 +1234,7 @@ static int64_t get_file_size(int fd)
 }
 
 static void *file_ram_alloc(RAMBlock *block,
-                            ram_addr_t memory,
+                            ram_addr_t *memory,
                             const char *path,
                             Error **errp)
 {
@@ -1245,6 +1245,7 @@ static void *file_ram_alloc(RAMBlock *block,
     void *area = MAP_FAILED;
     int fd = -1;
     int64_t file_size;
+    ram_addr_t mem_size = *memory;
 
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
         error_setg(errp,
@@ -1309,21 +1310,27 @@ static void *file_ram_alloc(RAMBlock *block,
 
     file_size = get_file_size(fd);
 
-    if (memory < block->page_size) {
+    if (!mem_size && file_size > 0) {
+        mem_size = file_size;
+        memory_region_set_size(block->mr, mem_size);
+    }
+
+    if (mem_size < block->page_size) {
         error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
                    "or larger than page size 0x%zx",
-                   memory, block->page_size);
+                   mem_size, block->page_size);
         goto error;
     }
 
-    if (file_size > 0 && file_size < memory) {
+    if (file_size > 0 && file_size < mem_size) {
         error_setg(errp, "backing store %s size %"PRId64
                    " does not match 'size' option %"PRIu64,
-                   path, file_size, memory);
+                   path, file_size, mem_size);
         goto error;
     }
 
-    memory = ROUND_UP(memory, block->page_size);
+    mem_size = ROUND_UP(mem_size, block->page_size);
+    *memory = mem_size;
 
     /*
      * ftruncate is not supported by hugetlbfs in older
@@ -1339,11 +1346,11 @@ static void *file_ram_alloc(RAMBlock *block,
      * those labels. Therefore, extending the non-empty backend file
      * is disabled as well.
      */
-    if (!file_size && ftruncate(fd, memory)) {
+    if (!file_size && ftruncate(fd, mem_size)) {
         perror("ftruncate");
     }
 
-    area = qemu_ram_mmap(fd, memory, block->mr->align,
+    area = qemu_ram_mmap(fd, mem_size, block->mr->align,
                          block->flags & RAM_SHARED);
     if (area == MAP_FAILED) {
         error_setg_errno(errp, errno,
@@ -1352,7 +1359,7 @@ static void *file_ram_alloc(RAMBlock *block,
     }
 
     if (mem_prealloc) {
-        os_mem_prealloc(fd, area, memory, errp);
+        os_mem_prealloc(fd, area, mem_size, errp);
         if (errp && *errp) {
             goto error;
         }
@@ -1363,7 +1370,7 @@ static void *file_ram_alloc(RAMBlock *block,
 
 error:
     if (area != MAP_FAILED) {
-        qemu_ram_munmap(area, memory);
+        qemu_ram_munmap(area, mem_size);
     }
     if (unlink_on_error) {
         unlink(path);
@@ -1690,15 +1697,15 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
     size = HOST_PAGE_ALIGN(size);
     new_block = g_malloc0(sizeof(*new_block));
     new_block->mr = mr;
-    new_block->used_length = size;
-    new_block->max_length = size;
     new_block->flags = share ? RAM_SHARED : 0;
-    new_block->host = file_ram_alloc(new_block, size,
+    new_block->host = file_ram_alloc(new_block, &size,
                                      mem_path, errp);
     if (!new_block->host) {
         g_free(new_block);
         return NULL;
     }
+    new_block->used_length = size;
+    new_block->max_length = size;
 
     ram_block_add(new_block, &local_err);
     if (local_err) {
-- 
2.10.1

Re: [Qemu-devel] [PATCH v2 0/3] Improve truncation behavior of memory-backend-file

[Paolo Bonzini]

On 27/10/2016 06:22, Haozhong Zhang wrote:
> For a mmeory backend file, e.g.
>     -object memory-backend-file,mem-path=foo,size=SZ,...
> the size of the backend file 'foo' is specified by the 'size'
> option. If the specified size 'SZ' does not match the actual size of
> file 'foo', QEMU will truncate the backend file 'foo'. In certain
> usage scenarios (e.g. vNVDIMM), the truncation may corrupt the
> existing data in the file.
> 
> Patch 1 in this series avoids such data corruption by disabling
> truncating non-empty backend files. If a non-existing file, an empty
> file or a directory is specified by 'mem-path' option, QEMU will
> truncate the backend file to the size specified by 'size' option.
> 
> Patch 2 adds an additional check to avoid creating a memory backend
> that can not be hold in the backend file. For a non-empty backend
> file, if its size is smaller than 'size' option, QEMU will report
> error.
> 
> Patch 3 makes the option 'size' optional. It's to avoid the misusing
> of 'size' option. If the user is uncertain about the backend file
> size, they can skip the 'size' option and let QEMU use the actual file
> size.  If a non-existing file, an empty file or a directory is
> specified by 'mem-path' option, the 'size' option is still needed.
> 
> Changes since v1:
>  * Fix errors in v1 patches.
>  * Split truncation skip and size check into separate patches.
>  * Do not error out for backend file whose size is unknown.
>  * Only error out when file size is smaller than 'size' option.
>  * Change the error handling path of file_backend_memory_alloc().
>  * Do not duplicate the setting of block->used_length/max_length in
>    file_ram_alloc().

Nice.  I'm squashing this in for slightly better error messages.

Thanks,

Paolo

diff --git a/exec.c b/exec.c
index d9034b1..eea9c10 100644
--- a/exec.c
+++ b/exec.c
@@ -1267,6 +1267,13 @@ static void *file_ram_alloc(RAMBlock *block,
                 break;
             }
         } else if (errno == EISDIR) {
+            if (!mem_size) {
+                error_setg_errno(errp, errno,
+                                 "%s is a directory but no 'size' option was specified",
+                                 path);
+                goto error;
+            }
+
             /* @path names a directory, create a file there */
             /* Make name safe to use with mkstemp by replacing '/' with '_'. */
             sanitized_name = g_strdup(memory_region_name(block->mr));

> 
> Haozhong Zhang (3):
>   exec.c: do not truncate non-empty memory backend file
>   exec.c: check memory backend file size with 'size' option
>   hostmem-file: make option 'size' optional
> 
>  backends/hostmem-file.c | 28 ++++++++++++++++++-------
>  exec.c                  | 56 +++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 65 insertions(+), 19 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 1/3] exec.c: do not truncate non-empty memory backend file

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 12:22:58PM +0800, Haozhong Zhang wrote:
> For '-object memory-backend-file,mem-path=foo,size=xyz', if the size of
> file 'foo' does not match the given size 'xyz', the current QEMU will
> truncate the file to the given size, which may corrupt the existing data
> in that file. To avoid such data corruption, this patch disables
> truncating non-empty backend files.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But I would add comment near the get_file_size() call to indicate
that not stopping on get_file_size() errors is on purpose and not
a mistake.

> ---
>  exec.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index 587b489..a2b371a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1224,6 +1224,15 @@ void qemu_mutex_unlock_ramlist(void)
>  }
>  
>  #ifdef __linux__
> +static int64_t get_file_size(int fd)
> +{
> +    int64_t size = lseek(fd, 0, SEEK_END);
> +    if (size < 0) {
> +        return -errno;
> +    }
> +    return size;
> +}
> +
>  static void *file_ram_alloc(RAMBlock *block,
>                              ram_addr_t memory,
>                              const char *path,
> @@ -1235,6 +1244,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      char *c;
>      void *area = MAP_FAILED;
>      int fd = -1;
> +    int64_t file_size;
>  
>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
>          error_setg(errp,
> @@ -1297,6 +1307,8 @@ static void *file_ram_alloc(RAMBlock *block,
>      }
>  #endif
>  
> +    file_size = get_file_size(fd);
> +
>      if (memory < block->page_size) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
>                     "or larger than page size 0x%zx",
> @@ -1311,8 +1323,16 @@ static void *file_ram_alloc(RAMBlock *block,
>       * hosts, so don't bother bailing out on errors.
>       * If anything goes wrong with it under other filesystems,
>       * mmap will fail.
> +     *
> +     * Do not truncate the non-empty backend file to avoid corrupting
> +     * the existing data in the file. Disabling shrinking is not
> +     * enough. For example, the current vNVDIMM implementation stores
> +     * the guest NVDIMM labels at the end of the backend file. If the
> +     * backend file is later extended, QEMU will not be able to find
> +     * those labels. Therefore, extending the non-empty backend file
> +     * is disabled as well.
>       */
> -    if (ftruncate(fd, memory)) {
> +    if (!file_size && ftruncate(fd, memory)) {
>          perror("ftruncate");
>      }
>  
> -- 
> 2.10.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/3] exec.c: check memory backend file size with 'size' option

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 12:22:59PM +0800, Haozhong Zhang wrote:
> If the memory backend file is not large enough to hold the required 'size',
> Qemu will report error and exit.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  exec.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index a2b371a..264a25f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1316,6 +1316,13 @@ static void *file_ram_alloc(RAMBlock *block,
>          goto error;
>      }
>  
> +    if (file_size > 0 && file_size < memory) {
> +        error_setg(errp, "backing store %s size %"PRId64
> +                   " does not match 'size' option %"PRIu64,
> +                   path, file_size, memory);
> +        goto error;
> +    }
> +
>      memory = ROUND_UP(memory, block->page_size);
>  
>      /*
> -- 
> 2.10.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optional

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 12:23:00PM +0800, Haozhong Zhang wrote:
> If 'size' option is not given, Qemu will use the file size of 'mem-path'
> instead. If an empty file, a non-existing file or a directory is specified
> by option 'mem-path', a non-zero option 'size' is still needed.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  backends/hostmem-file.c | 28 ++++++++++++++++++++--------
>  exec.c                  | 33 ++++++++++++++++++++-------------
>  2 files changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 42efb2f..6ee4352 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -39,17 +39,14 @@ static void
>  file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>  {
>      HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
> +    Error *local_err = NULL;
>  
> -    if (!backend->size) {
> -        error_setg(errp, "can't create backend with size 0");
> -        return;
> -    }
>      if (!fb->mem_path) {
> -        error_setg(errp, "mem-path property not set");
> -        return;
> +        error_setg(&local_err, "mem-path property not set");
> +        goto out;
>      }
>  #ifndef CONFIG_LINUX
> -    error_setg(errp, "-mem-path not supported on this host");
> +    error_setg(&local_err, "-mem-path not supported on this host");
>  #else
>      if (!memory_region_size(&backend->mr)) {
>          gchar *path;
> @@ -58,10 +55,25 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>          memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>                                   path,
>                                   backend->size, fb->share,
> -                                 fb->mem_path, errp);
> +                                 fb->mem_path, &local_err);
>          g_free(path);
> +
> +        if (local_err) {
> +            goto out;
> +        }
> +
> +        if (!backend->size) {
> +            backend->size = memory_region_size(&backend->mr);
> +        }
>      }
>  #endif
> +
> +    if (!backend->size) {
> +        error_setg(&local_err, "can't create backend with size 0");
> +    }

You need to move this check before the #endif line, as an error
is already unconditionally set in local_err in the !CONFIG_LINUX
path, and a second error_setg() call would trigger an assert()
inside error_setv().

> +
> + out:
> +    error_propagate(errp, local_err);
>  }
>  
>  static char *get_mem_path(Object *o, Error **errp)
> diff --git a/exec.c b/exec.c
> index 264a25f..89065bd 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1234,7 +1234,7 @@ static int64_t get_file_size(int fd)
>  }
>  
>  static void *file_ram_alloc(RAMBlock *block,
> -                            ram_addr_t memory,
> +                            ram_addr_t *memory,
>                              const char *path,
>                              Error **errp)
>  {
> @@ -1245,6 +1245,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      void *area = MAP_FAILED;
>      int fd = -1;
>      int64_t file_size;
> +    ram_addr_t mem_size = *memory;
>  
>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
>          error_setg(errp,
> @@ -1309,21 +1310,27 @@ static void *file_ram_alloc(RAMBlock *block,
>  
>      file_size = get_file_size(fd);
>  
> -    if (memory < block->page_size) {
> +    if (!mem_size && file_size > 0) {
> +        mem_size = file_size;

Maybe we should set *memory here and not below?

> +        memory_region_set_size(block->mr, mem_size);

This could be simplified (and made safer) if the memory region
got initialized by memory_region_init_ram_from_file() after we
already mapped/allocated the file (so we avoid surprises in case
other code does something weird because of the temporarily
zero-sized MemoryRegion). But it would probably be an intrusive
change, so I guess changing the memory size here is OK. Paolo,
what do you think?

> +    }
> +
> +    if (mem_size < block->page_size) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
>                     "or larger than page size 0x%zx",
> -                   memory, block->page_size);
> +                   mem_size, block->page_size);
>          goto error;
>      }
>  
> -    if (file_size > 0 && file_size < memory) {
> +    if (file_size > 0 && file_size < mem_size) {
>          error_setg(errp, "backing store %s size %"PRId64
>                     " does not match 'size' option %"PRIu64,
> -                   path, file_size, memory);
> +                   path, file_size, mem_size);
>          goto error;
>      }
>  
> -    memory = ROUND_UP(memory, block->page_size);
> +    mem_size = ROUND_UP(mem_size, block->page_size);
> +    *memory = mem_size;

Why exactly did you choose to set *memory to the rounded-up size
and not file_size? Changing *memory to the rounded-up value would
be additional behavior that is not described in the commit
message. I believe we should change *memory only if *memory == 0,
and avoid touching it otherwise.

>  
>      /*
>       * ftruncate is not supported by hugetlbfs in older
> @@ -1339,11 +1346,11 @@ static void *file_ram_alloc(RAMBlock *block,
>       * those labels. Therefore, extending the non-empty backend file
>       * is disabled as well.
>       */
> -    if (!file_size && ftruncate(fd, memory)) {
> +    if (!file_size && ftruncate(fd, mem_size)) {
>          perror("ftruncate");
>      }
>  
> -    area = qemu_ram_mmap(fd, memory, block->mr->align,
> +    area = qemu_ram_mmap(fd, mem_size, block->mr->align,
>                           block->flags & RAM_SHARED);
>      if (area == MAP_FAILED) {
>          error_setg_errno(errp, errno,
> @@ -1352,7 +1359,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      }
>  
>      if (mem_prealloc) {
> -        os_mem_prealloc(fd, area, memory, errp);
> +        os_mem_prealloc(fd, area, mem_size, errp);
>          if (errp && *errp) {
>              goto error;
>          }
> @@ -1363,7 +1370,7 @@ static void *file_ram_alloc(RAMBlock *block,
>  
>  error:
>      if (area != MAP_FAILED) {
> -        qemu_ram_munmap(area, memory);
> +        qemu_ram_munmap(area, mem_size);
>      }
>      if (unlink_on_error) {
>          unlink(path);
> @@ -1690,15 +1697,15 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>      size = HOST_PAGE_ALIGN(size);
>      new_block = g_malloc0(sizeof(*new_block));
>      new_block->mr = mr;
> -    new_block->used_length = size;
> -    new_block->max_length = size;
>      new_block->flags = share ? RAM_SHARED : 0;
> -    new_block->host = file_ram_alloc(new_block, size,
> +    new_block->host = file_ram_alloc(new_block, &size,
>                                       mem_path, errp);
>      if (!new_block->host) {
>          g_free(new_block);
>          return NULL;
>      }
> +    new_block->used_length = size;
> +    new_block->max_length = size;
>  
>      ram_block_add(new_block, &local_err);
>      if (local_err) {
> -- 
> 2.10.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optional

[Haozhong Zhang]

On 10/27/16 12:55 -0200, Eduardo Habkost wrote:
>On Thu, Oct 27, 2016 at 12:23:00PM +0800, Haozhong Zhang wrote:
>> If 'size' option is not given, Qemu will use the file size of 'mem-path'
>> instead. If an empty file, a non-existing file or a directory is specified
>> by option 'mem-path', a non-zero option 'size' is still needed.
>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> ---
>>  backends/hostmem-file.c | 28 ++++++++++++++++++++--------
>>  exec.c                  | 33 ++++++++++++++++++++-------------
>>  2 files changed, 40 insertions(+), 21 deletions(-)
>>
>> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>> index 42efb2f..6ee4352 100644
>> --- a/backends/hostmem-file.c
>> +++ b/backends/hostmem-file.c
>> @@ -39,17 +39,14 @@ static void
>>  file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>  {
>>      HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>> +    Error *local_err = NULL;
>>
>> -    if (!backend->size) {
>> -        error_setg(errp, "can't create backend with size 0");
>> -        return;
>> -    }
>>      if (!fb->mem_path) {
>> -        error_setg(errp, "mem-path property not set");
>> -        return;
>> +        error_setg(&local_err, "mem-path property not set");
>> +        goto out;
>>      }
>>  #ifndef CONFIG_LINUX
>> -    error_setg(errp, "-mem-path not supported on this host");
>> +    error_setg(&local_err, "-mem-path not supported on this host");
>>  #else
>>      if (!memory_region_size(&backend->mr)) {
>>          gchar *path;
>> @@ -58,10 +55,25 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>          memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
>>                                   path,
>>                                   backend->size, fb->share,
>> -                                 fb->mem_path, errp);
>> +                                 fb->mem_path, &local_err);
>>          g_free(path);
>> +
>> +        if (local_err) {
>> +            goto out;
>> +        }
>> +
>> +        if (!backend->size) {
>> +            backend->size = memory_region_size(&backend->mr);
>> +        }
>>      }
>>  #endif
>> +
>> +    if (!backend->size) {
>> +        error_setg(&local_err, "can't create backend with size 0");
>> +    }
>
>You need to move this check before the #endif line, as an error
>is already unconditionally set in local_err in the !CONFIG_LINUX
>path, and a second error_setg() call would trigger an assert()
>inside error_setv().
>

will change

>> +
>> + out:
>> +    error_propagate(errp, local_err);
>>  }
>>
>>  static char *get_mem_path(Object *o, Error **errp)
>> diff --git a/exec.c b/exec.c
>> index 264a25f..89065bd 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1234,7 +1234,7 @@ static int64_t get_file_size(int fd)
>>  }
>>
>>  static void *file_ram_alloc(RAMBlock *block,
>> -                            ram_addr_t memory,
>> +                            ram_addr_t *memory,
>>                              const char *path,
>>                              Error **errp)
>>  {
>> @@ -1245,6 +1245,7 @@ static void *file_ram_alloc(RAMBlock *block,
>>      void *area = MAP_FAILED;
>>      int fd = -1;
>>      int64_t file_size;
>> +    ram_addr_t mem_size = *memory;
>>
>>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
>>          error_setg(errp,
>> @@ -1309,21 +1310,27 @@ static void *file_ram_alloc(RAMBlock *block,
>>
>>      file_size = get_file_size(fd);
>>
>> -    if (memory < block->page_size) {
>> +    if (!mem_size && file_size > 0) {
>> +        mem_size = file_size;
>
>Maybe we should set *memory here and not below?
>

Qemu currently sets the memory region size to the file size, and block
length to the aligned file size, so the code here can be changed as below:

            memory_region_set_size(block->mr, mem_size);
            mem_size = HOST_PAGE_ALIGN(mem_size);
            *memory = mem_size;

The second line is necessary because Qemu currently passes the aligned
file size to file_ram_alloc().

>> +        memory_region_set_size(block->mr, mem_size);
>
>This could be simplified (and made safer) if the memory region
>got initialized by memory_region_init_ram_from_file() after we
>already mapped/allocated the file (so we avoid surprises in case
>other code does something weird because of the temporarily
>zero-sized MemoryRegion). But it would probably be an intrusive
>change, so I guess changing the memory size here is OK. Paolo,
>what do you think?
>

I will add a check to ensure that the size of block->mr is either 0 or
mem_size, and only set the region size in the former case. The former
corresponds to the case that 'size' option is not given or 0. The
latter corresponds to the case that 'size' option is given.

>> +    }
>> +
>> +    if (mem_size < block->page_size) {
>>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
>>                     "or larger than page size 0x%zx",
>> -                   memory, block->page_size);
>> +                   mem_size, block->page_size);
>>          goto error;
>>      }
>>
>> -    if (file_size > 0 && file_size < memory) {
>> +    if (file_size > 0 && file_size < mem_size) {
>>          error_setg(errp, "backing store %s size %"PRId64
>>                     " does not match 'size' option %"PRIu64,
>> -                   path, file_size, memory);
>> +                   path, file_size, mem_size);
>>          goto error;
>>      }
>>
>> -    memory = ROUND_UP(memory, block->page_size);
>> +    mem_size = ROUND_UP(mem_size, block->page_size);
>> +    *memory = mem_size;
>
>Why exactly did you choose to set *memory to the rounded-up size
>and not file_size? Changing *memory to the rounded-up value would
>be additional behavior that is not described in the commit
>message. I believe we should change *memory only if *memory == 0,
>and avoid touching it otherwise.
>

Setting *memory here is buggy. I'll move it as above.

Thanks,
Haozhong

>>
>>      /*
>>       * ftruncate is not supported by hugetlbfs in older
>> @@ -1339,11 +1346,11 @@ static void *file_ram_alloc(RAMBlock *block,
>>       * those labels. Therefore, extending the non-empty backend file
>>       * is disabled as well.
>>       */
>> -    if (!file_size && ftruncate(fd, memory)) {
>> +    if (!file_size && ftruncate(fd, mem_size)) {
>>          perror("ftruncate");
>>      }
>>
>> -    area = qemu_ram_mmap(fd, memory, block->mr->align,
>> +    area = qemu_ram_mmap(fd, mem_size, block->mr->align,
>>                           block->flags & RAM_SHARED);
>>      if (area == MAP_FAILED) {
>>          error_setg_errno(errp, errno,
>> @@ -1352,7 +1359,7 @@ static void *file_ram_alloc(RAMBlock *block,
>>      }
>>
>>      if (mem_prealloc) {
>> -        os_mem_prealloc(fd, area, memory, errp);
>> +        os_mem_prealloc(fd, area, mem_size, errp);
>>          if (errp && *errp) {
>>              goto error;
>>          }
>> @@ -1363,7 +1370,7 @@ static void *file_ram_alloc(RAMBlock *block,
>>
>>  error:
>>      if (area != MAP_FAILED) {
>> -        qemu_ram_munmap(area, memory);
>> +        qemu_ram_munmap(area, mem_size);
>>      }
>>      if (unlink_on_error) {
>>          unlink(path);
>> @@ -1690,15 +1697,15 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>>      size = HOST_PAGE_ALIGN(size);
>>      new_block = g_malloc0(sizeof(*new_block));
>>      new_block->mr = mr;
>> -    new_block->used_length = size;
>> -    new_block->max_length = size;
>>      new_block->flags = share ? RAM_SHARED : 0;
>> -    new_block->host = file_ram_alloc(new_block, size,
>> +    new_block->host = file_ram_alloc(new_block, &size,
>>                                       mem_path, errp);
>>      if (!new_block->host) {
>>          g_free(new_block);
>>          return NULL;
>>      }
>> +    new_block->used_length = size;
>> +    new_block->max_length = size;
>>
>>      ram_block_add(new_block, &local_err);
>>      if (local_err) {
>> --
>> 2.10.1
>>
>
>-- 
>Eduardo

Re: [Qemu-devel] [PATCH v2 1/3] exec.c: do not truncate non-empty memory backend file

[Haozhong Zhang]

On 10/27/16 12:31 -0200, Eduardo Habkost wrote:
>On Thu, Oct 27, 2016 at 12:22:58PM +0800, Haozhong Zhang wrote:
>> For '-object memory-backend-file,mem-path=foo,size=xyz', if the size of
>> file 'foo' does not match the given size 'xyz', the current QEMU will
>> truncate the file to the given size, which may corrupt the existing data
>> in that file. To avoid such data corruption, this patch disables
>> truncating non-empty backend files.
>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>
>Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
>But I would add comment near the get_file_size() call to indicate
>that not stopping on get_file_size() errors is on purpose and not
>a mistake.
>

I'll add comments in the next version.

Thanks,
Haozhong

>> ---
>>  exec.c | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 587b489..a2b371a 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1224,6 +1224,15 @@ void qemu_mutex_unlock_ramlist(void)
>>  }
>>
>>  #ifdef __linux__
>> +static int64_t get_file_size(int fd)
>> +{
>> +    int64_t size = lseek(fd, 0, SEEK_END);
>> +    if (size < 0) {
>> +        return -errno;
>> +    }
>> +    return size;
>> +}
>> +
>>  static void *file_ram_alloc(RAMBlock *block,
>>                              ram_addr_t memory,
>>                              const char *path,
>> @@ -1235,6 +1244,7 @@ static void *file_ram_alloc(RAMBlock *block,
>>      char *c;
>>      void *area = MAP_FAILED;
>>      int fd = -1;
>> +    int64_t file_size;
>>
>>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
>>          error_setg(errp,
>> @@ -1297,6 +1307,8 @@ static void *file_ram_alloc(RAMBlock *block,
>>      }
>>  #endif
>>
>> +    file_size = get_file_size(fd);
>> +
>>      if (memory < block->page_size) {
>>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
>>                     "or larger than page size 0x%zx",
>> @@ -1311,8 +1323,16 @@ static void *file_ram_alloc(RAMBlock *block,
>>       * hosts, so don't bother bailing out on errors.
>>       * If anything goes wrong with it under other filesystems,
>>       * mmap will fail.
>> +     *
>> +     * Do not truncate the non-empty backend file to avoid corrupting
>> +     * the existing data in the file. Disabling shrinking is not
>> +     * enough. For example, the current vNVDIMM implementation stores
>> +     * the guest NVDIMM labels at the end of the backend file. If the
>> +     * backend file is later extended, QEMU will not be able to find
>> +     * those labels. Therefore, extending the non-empty backend file
>> +     * is disabled as well.
>>       */
>> -    if (ftruncate(fd, memory)) {
>> +    if (!file_size && ftruncate(fd, memory)) {
>>          perror("ftruncate");
>>      }
>>
>> --
>> 2.10.1
>>
>
>-- 
>Eduardo

Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optional

[Haozhong Zhang]

On 10/28/16 10:06 +0800, Haozhong Zhang wrote:
>On 10/27/16 12:55 -0200, Eduardo Habkost wrote:
>>On Thu, Oct 27, 2016 at 12:23:00PM +0800, Haozhong Zhang wrote:
[..]
>>> static char *get_mem_path(Object *o, Error **errp)
>>>diff --git a/exec.c b/exec.c
>>>index 264a25f..89065bd 100644
>>>--- a/exec.c
>>>+++ b/exec.c
>>>@@ -1234,7 +1234,7 @@ static int64_t get_file_size(int fd)
>>> }
>>>
>>> static void *file_ram_alloc(RAMBlock *block,
>>>-                            ram_addr_t memory,
>>>+                            ram_addr_t *memory,
>>>                             const char *path,
>>>                             Error **errp)
>>> {
>>>@@ -1245,6 +1245,7 @@ static void *file_ram_alloc(RAMBlock *block,
>>>     void *area = MAP_FAILED;
>>>     int fd = -1;
>>>     int64_t file_size;
>>>+    ram_addr_t mem_size = *memory;
>>>
>>>     if (kvm_enabled() && !kvm_has_sync_mmu()) {
>>>         error_setg(errp,
>>>@@ -1309,21 +1310,27 @@ static void *file_ram_alloc(RAMBlock *block,
>>>
>>>     file_size = get_file_size(fd);
>>>
>>>-    if (memory < block->page_size) {
>>>+    if (!mem_size && file_size > 0) {
>>>+        mem_size = file_size;
>>
>>Maybe we should set *memory here and not below?
>>
>
>Qemu currently sets the memory region size to the file size, and block
>length to the aligned file size, so the code here can be changed as below:
>
>           memory_region_set_size(block->mr, mem_size);
>           mem_size = HOST_PAGE_ALIGN(mem_size);
>           *memory = mem_size;
>
>The second line is necessary because Qemu currently passes the aligned
>file size to file_ram_alloc().
>

I meant that QEMU currently sets the memory region size of the 'size'
option and the block length to the aligned 'size' option. If the file
size is used when 'size' option is not specified, QEMU should do the
same thing.

Haozhong

>>>+        memory_region_set_size(block->mr, mem_size);
>>
>>This could be simplified (and made safer) if the memory region
>>got initialized by memory_region_init_ram_from_file() after we
>>already mapped/allocated the file (so we avoid surprises in case
>>other code does something weird because of the temporarily
>>zero-sized MemoryRegion). But it would probably be an intrusive
>>change, so I guess changing the memory size here is OK. Paolo,
>>what do you think?
>>
>
>I will add a check to ensure that the size of block->mr is either 0 or
>mem_size, and only set the region size in the former case. The former
>corresponds to the case that 'size' option is not given or 0. The
>latter corresponds to the case that 'size' option is given.
>
>>>+    }
>>>+
>>>+    if (mem_size < block->page_size) {
>>>         error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
>>>                    "or larger than page size 0x%zx",
>>>-                   memory, block->page_size);
>>>+                   mem_size, block->page_size);
>>>         goto error;
>>>     }
>>>
>>>-    if (file_size > 0 && file_size < memory) {
>>>+    if (file_size > 0 && file_size < mem_size) {
>>>         error_setg(errp, "backing store %s size %"PRId64
>>>                    " does not match 'size' option %"PRIu64,
>>>-                   path, file_size, memory);
>>>+                   path, file_size, mem_size);
>>>         goto error;
>>>     }
>>>
>>>-    memory = ROUND_UP(memory, block->page_size);
>>>+    mem_size = ROUND_UP(mem_size, block->page_size);
>>>+    *memory = mem_size;
>>
>>Why exactly did you choose to set *memory to the rounded-up size
>>and not file_size? Changing *memory to the rounded-up value would
>>be additional behavior that is not described in the commit
>>message. I believe we should change *memory only if *memory == 0,
>>and avoid touching it otherwise.
>>
>
>Setting *memory here is buggy. I'll move it as above.
>
>Thanks,
>Haozhong
>
[..]

Re: [Qemu-devel] [PATCH v2 1/3] exec.c: do not truncate non-empty memory backend file

[Eduardo Habkost]

On Fri, Oct 28, 2016 at 10:07:40AM +0800, Haozhong Zhang wrote:
> On 10/27/16 12:31 -0200, Eduardo Habkost wrote:
> > On Thu, Oct 27, 2016 at 12:22:58PM +0800, Haozhong Zhang wrote:
> > > For '-object memory-backend-file,mem-path=foo,size=xyz', if the size of
> > > file 'foo' does not match the given size 'xyz', the current QEMU will
> > > truncate the file to the given size, which may corrupt the existing data
> > > in that file. To avoid such data corruption, this patch disables
> > > truncating non-empty backend files.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > But I would add comment near the get_file_size() call to indicate
> > that not stopping on get_file_size() errors is on purpose and not
> > a mistake.
> > 
> 
> I'll add comments in the next version.
> 

The patch was applied to machine-next and will be in my next pull
request. The comment can be sent as a follow-up patch.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/3] exec.c: check memory backend file size with 'size' option

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 12:22:59PM +0800, Haozhong Zhang wrote:
> If the memory backend file is not large enough to hold the required 'size',
> Qemu will report error and exit.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Applied to machine-next. Thanks!

> ---
>  exec.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index a2b371a..264a25f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1316,6 +1316,13 @@ static void *file_ram_alloc(RAMBlock *block,
>          goto error;
>      }
>  
> +    if (file_size > 0 && file_size < memory) {
> +        error_setg(errp, "backing store %s size %"PRId64
> +                   " does not match 'size' option %"PRIu64,
> +                   path, file_size, memory);
> +        goto error;
> +    }
> +
>      memory = ROUND_UP(memory, block->page_size);
>  
>      /*
> -- 
> 2.10.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/3] exec.c: check memory backend file size with 'size' option

[Paolo Bonzini]

On 31/10/2016 18:23, Eduardo Habkost wrote:
> On Thu, Oct 27, 2016 at 12:22:59PM +0800, Haozhong Zhang wrote:
>> If the memory backend file is not large enough to hold the required 'size',
>> Qemu will report error and exit.
>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> Applied to machine-next. Thanks!

Also part of my pull request. :)

Paolo

>> ---
>>  exec.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/exec.c b/exec.c
>> index a2b371a..264a25f 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1316,6 +1316,13 @@ static void *file_ram_alloc(RAMBlock *block,
>>          goto error;
>>      }
>>  
>> +    if (file_size > 0 && file_size < memory) {
>> +        error_setg(errp, "backing store %s size %"PRId64
>> +                   " does not match 'size' option %"PRIu64,
>> +                   path, file_size, memory);
>> +        goto error;
>> +    }
>> +
>>      memory = ROUND_UP(memory, block->page_size);
>>  
>>      /*
>> -- 
>> 2.10.1
>>
>>
> 

Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optional

[Eduardo Habkost]

On Fri, Oct 28, 2016 at 10:06:40AM +0800, Haozhong Zhang wrote:
[...]
> > > diff --git a/exec.c b/exec.c
> > > index 264a25f..89065bd 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -1234,7 +1234,7 @@ static int64_t get_file_size(int fd)
> > >  }
> > > 
> > >  static void *file_ram_alloc(RAMBlock *block,
> > > -                            ram_addr_t memory,
> > > +                            ram_addr_t *memory,
> > >                              const char *path,
> > >                              Error **errp)
> > >  {
> > > @@ -1245,6 +1245,7 @@ static void *file_ram_alloc(RAMBlock *block,
> > >      void *area = MAP_FAILED;
> > >      int fd = -1;
> > >      int64_t file_size;
> > > +    ram_addr_t mem_size = *memory;
> > > 
> > >      if (kvm_enabled() && !kvm_has_sync_mmu()) {
> > >          error_setg(errp,
> > > @@ -1309,21 +1310,27 @@ static void *file_ram_alloc(RAMBlock *block,
> > > 
> > >      file_size = get_file_size(fd);
> > > 
> > > -    if (memory < block->page_size) {
> > > +    if (!mem_size && file_size > 0) {
> > > +        mem_size = file_size;
> > 
> > Maybe we should set *memory here and not below?
> > 
> 
> Qemu currently sets the memory region size to the file size, and block
> length to the aligned file size, so the code here can be changed as below:
> 
>            memory_region_set_size(block->mr, mem_size);
>            mem_size = HOST_PAGE_ALIGN(mem_size);
>            *memory = mem_size;
> 
> The second line is necessary because Qemu currently passes the aligned
> file size to file_ram_alloc().

That would duplicate the existing HOST_PAGE_ALIGN logic from
qemu_ram_alloc_from_file(), won't it?

I believe that's yet another reason to check file size before
initializing the memory region, instead of initializing it first,
and fixing up its size later.

-- 
Eduardo

[Qemu-devel] [RESEND PATCH v2 2/3] exec.c: check memory backend file size with 'size' option

[Haozhong Zhang]

If the memory backend file is not large enough to hold the required 'size',
Qemu will report error and exit.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Message-Id: <20161027042300.5929-3-haozhong.zhang@intel.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes in RESEND:
* Use format string RAM_ADDR_FMT for variable 'memory'.
  This change is to fix the build error.
* Add Eduardo's r-b.
* Add the message id of this thread.
---
 exec.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/exec.c b/exec.c
index a2b371a..23c21c2 100644
--- a/exec.c
+++ b/exec.c
@@ -1316,6 +1316,13 @@ static void *file_ram_alloc(RAMBlock *block,
         goto error;
     }
 
+    if (file_size > 0 && file_size < memory) {
+        error_setg(errp, "backing store %s size 0x%" PRIx64
+                   " does not match 'size' option 0x" RAM_ADDR_FMT,
+                   path, file_size, memory);
+        goto error;
+    }
+
     memory = ROUND_UP(memory, block->page_size);
 
     /*
-- 
2.10.1

Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: make option 'size' optional

[Haozhong Zhang]

On 10/31/16 16:18 -0200, Eduardo Habkost wrote:
>On Fri, Oct 28, 2016 at 10:06:40AM +0800, Haozhong Zhang wrote:
>[...]
>> > > diff --git a/exec.c b/exec.c
>> > > index 264a25f..89065bd 100644
>> > > --- a/exec.c
>> > > +++ b/exec.c
>> > > @@ -1234,7 +1234,7 @@ static int64_t get_file_size(int fd)
>> > >  }
>> > >
>> > >  static void *file_ram_alloc(RAMBlock *block,
>> > > -                            ram_addr_t memory,
>> > > +                            ram_addr_t *memory,
>> > >                              const char *path,
>> > >                              Error **errp)
>> > >  {
>> > > @@ -1245,6 +1245,7 @@ static void *file_ram_alloc(RAMBlock *block,
>> > >      void *area = MAP_FAILED;
>> > >      int fd = -1;
>> > >      int64_t file_size;
>> > > +    ram_addr_t mem_size = *memory;
>> > >
>> > >      if (kvm_enabled() && !kvm_has_sync_mmu()) {
>> > >          error_setg(errp,
>> > > @@ -1309,21 +1310,27 @@ static void *file_ram_alloc(RAMBlock *block,
>> > >
>> > >      file_size = get_file_size(fd);
>> > >
>> > > -    if (memory < block->page_size) {
>> > > +    if (!mem_size && file_size > 0) {
>> > > +        mem_size = file_size;
>> >
>> > Maybe we should set *memory here and not below?
>> >
>>
>> Qemu currently sets the memory region size to the file size, and block
>> length to the aligned file size, so the code here can be changed as below:
>>
>>            memory_region_set_size(block->mr, mem_size);
>>            mem_size = HOST_PAGE_ALIGN(mem_size);
>>            *memory = mem_size;
>>
>> The second line is necessary because Qemu currently passes the aligned
>> file size to file_ram_alloc().
>
>That would duplicate the existing HOST_PAGE_ALIGN logic from
>qemu_ram_alloc_from_file(), won't it?
>
>I believe that's yet another reason to check file size before
>initializing the memory region, instead of initializing it first,
>and fixing up its size later.
>

Agree. In the next version of this patch 3, I'll move file operations
(open/close/get size) from file_ram_alloc() to qemu_ram_alloc_from_file().

Thanks,
Haozhong