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

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

[Haozhong Zhang]

The previous discussion thread can be found at
http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04668.html

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 the backend file is not empty
and the option 'size' does not match the actual file size, QEMU will
error out. If the backend file is empty, QEMU will extend to the size
specified by the option 'size'.

Patch 2 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.
For an empty file, the option 'size' must be specified.

Haozhong Zhang (2):
  exec.c: do not truncate non-empty memory backend file
  hostmem-file: allow option 'size' optional

 backends/hostmem-file.c | 10 ++++++----
 exec.c                  | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 5 deletions(-)

-- 
2.10.1

[Qemu-devel] [PATCH 1/2] 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 | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index e63c5a1..95983c9 100644
--- a/exec.c
+++ b/exec.c
@@ -1188,6 +1188,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,
@@ -1199,6 +1208,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,
@@ -1256,6 +1266,14 @@ static void *file_ram_alloc(RAMBlock *block,
     block->page_size = qemu_fd_getpagesize(fd);
     block->mr->align = MAX(block->page_size, QEMU_VMALLOC_ALIGN);
 
+    file_size = get_file_size(fd);
+    if (file_size < 0) {
+        error_setg_errno(errp, file_size,
+                         "can't get size of backing store %s",
+                         path);
+        goto error;
+    }
+
     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",
@@ -1266,12 +1284,29 @@ static void *file_ram_alloc(RAMBlock *block,
     memory = ROUND_UP(memory, block->page_size);
 
     /*
+     * Do not extend/shrink the backend file if it's not empty, or its
+     * size does not match the aligned 'size=xxx' option. Otherwise,
+     * it is possible to corrupt 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 (file_size && file_size != memory) {
+        error_setg(errp, "backing store %s size %"PRId64
+                   " does not math with aligned 'size' option %"PRIu64,
+                   path, file_size, memory);
+        goto error;
+    }
+    /*
      * ftruncate is not supported by hugetlbfs in older
      * hosts, so don't bother bailing out on errors.
      * If anything goes wrong with it under other filesystems,
      * mmap will fail.
      */
-    if (ftruncate(fd, memory)) {
+    if (!file_size && ftruncate(fd, memory)) {
         perror("ftruncate");
     }
 
-- 
2.10.1

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

[Eduardo Habkost]

On Mon, Oct 24, 2016 at 05:21:50PM +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>
> ---
>  exec.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index e63c5a1..95983c9 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1188,6 +1188,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,
> @@ -1199,6 +1208,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,
> @@ -1256,6 +1266,14 @@ static void *file_ram_alloc(RAMBlock *block,
>      block->page_size = qemu_fd_getpagesize(fd);
>      block->mr->align = MAX(block->page_size, QEMU_VMALLOC_ALIGN);
>  
> +    file_size = get_file_size(fd);
> +    if (file_size < 0) {
> +        error_setg_errno(errp, file_size,
> +                         "can't get size of backing store %s",
> +                         path);

What about block devices or filesystems where lseek(SEEK_END) is
not supported? They work today, and would break with this patch.

I suggest just continuing without any errors (and not truncating
the file) in case it is not possible to get the file size.

> +        goto error;
> +    }
> +
>      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",
> @@ -1266,12 +1284,29 @@ static void *file_ram_alloc(RAMBlock *block,
>      memory = ROUND_UP(memory, block->page_size);
>  
>      /*
> +     * Do not extend/shrink the backend file if it's not empty, or its
> +     * size does not match the aligned 'size=xxx' option. Otherwise,
> +     * it is possible to corrupt 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 (file_size && file_size != memory) {
> +        error_setg(errp, "backing store %s size %"PRId64
> +                   " does not math with aligned 'size' option %"PRIu64,

Did you mean "specified 'size' option"?

> +                   path, file_size, memory);

We already support size being smaller than the backing file and
people may rely on it, so we shouldn't change this behavior. This
can be changed to:
    if (file_size > 0 && file_size < memory)

I also suggest doing this check in a separate patch. The two
changes (skipping truncation of non-empty files and exiting on
size mismatch) don't depend on each other.

> +        goto error;
> +    }
> +    /*
>       * ftruncate is not supported by hugetlbfs in older
>       * hosts, so don't bother bailing out on errors.
>       * If anything goes wrong with it under other filesystems,
>       * mmap will fail.
>       */
> -    if (ftruncate(fd, memory)) {
> +    if (!file_size && ftruncate(fd, memory)) {
>          perror("ftruncate");
>      }
>  
> -- 
> 2.10.1
> 

-- 
Eduardo

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

[Haozhong Zhang]

On 10/25/16 17:30 -0200, Eduardo Habkost wrote:
>On Mon, Oct 24, 2016 at 05:21:50PM +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>
>> ---
>>  exec.c | 37 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/exec.c b/exec.c
>> index e63c5a1..95983c9 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1188,6 +1188,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,
>> @@ -1199,6 +1208,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,
>> @@ -1256,6 +1266,14 @@ static void *file_ram_alloc(RAMBlock *block,
>>      block->page_size = qemu_fd_getpagesize(fd);
>>      block->mr->align = MAX(block->page_size, QEMU_VMALLOC_ALIGN);
>>
>> +    file_size = get_file_size(fd);
>> +    if (file_size < 0) {
>> +        error_setg_errno(errp, file_size,
>> +                         "can't get size of backing store %s",
>> +                         path);
>
>What about block devices or filesystems where lseek(SEEK_END) is
>not supported? They work today, and would break with this patch.
>
>I suggest just continuing without any errors (and not truncating
>the file) in case it is not possible to get the file size.
>

If it fails to get file size, I'd fall back to the 'size' option. If
it's not zero, QEMU will not truncate the file. If 'memory' is zero,
QEMU will error-out a message like "cannot get file size, 'size'
option should be provided".

>> +        goto error;
>> +    }
>> +
>>      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",
>> @@ -1266,12 +1284,29 @@ static void *file_ram_alloc(RAMBlock *block,
>>      memory = ROUND_UP(memory, block->page_size);
>>
>>      /*
>> +     * Do not extend/shrink the backend file if it's not empty, or its
>> +     * size does not match the aligned 'size=xxx' option. Otherwise,
>> +     * it is possible to corrupt 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 (file_size && file_size != memory) {
>> +        error_setg(errp, "backing store %s size %"PRId64
>> +                   " does not math with aligned 'size' option %"PRIu64,
>
>Did you mean "specified 'size' option"?
>

I meant aligned, because the original 'size' option might have been
aligned to a value larger than file_size at this point. I'll use
'specified' in the next version.

>> +                   path, file_size, memory);
>
>We already support size being smaller than the backing file and
>people may rely on it, so we shouldn't change this behavior. This
>can be changed to:
>    if (file_size > 0 && file_size < memory)
>

will change

>I also suggest doing this check in a separate patch. The two
>changes (skipping truncation of non-empty files and exiting on
>size mismatch) don't depend on each other.
>

will do

Thanks,
Haozhong

>> +        goto error;
>> +    }
>> +    /*
>>       * ftruncate is not supported by hugetlbfs in older
>>       * hosts, so don't bother bailing out on errors.
>>       * If anything goes wrong with it under other filesystems,
>>       * mmap will fail.
>>       */
>> -    if (ftruncate(fd, memory)) {
>> +    if (!file_size && ftruncate(fd, memory)) {
>>          perror("ftruncate");
>>      }
>>
>> --
>> 2.10.1
>>
>
>-- 
>Eduardo

[Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional

[Haozhong Zhang]

If 'size' option of hostmem-file is not given, QEMU will use the file
size of 'mem-path' instead. For an empty file, a non-zero size must be
specified by the option 'size'.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 backends/hostmem-file.c | 10 ++++++----
 exec.c                  |  8 ++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 42efb2f..f94d2f7 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -40,10 +40,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
 
-    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;
@@ -62,6 +58,12 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         g_free(path);
     }
 #endif
+    if (!errp && !backend->size) {
+        backend->size = memory_region_size(&backend->mr);
+        if (!backend->size) {
+            error_setg(errp, "can't create backend with size 0");
+        }
+    }
 }
 
 static char *get_mem_path(Object *o, Error **errp)
diff --git a/exec.c b/exec.c
index 95983c9..91adc62 100644
--- a/exec.c
+++ b/exec.c
@@ -1274,6 +1274,14 @@ static void *file_ram_alloc(RAMBlock *block,
         goto error;
     }
 
+    if (memory) {
+        memory = memory ?: file_size;
+        memory_region_set_size(block->mr, memory);
+        memory = HOST_PAGE_ALIGN(memory);
+        block->used_length = memory;
+        block->max_length = memory;
+    }
+
     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",
-- 
2.10.1

Re: [Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional

[Eduardo Habkost]

On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote:
> If 'size' option of hostmem-file is not given, QEMU will use the file
> size of 'mem-path' instead. For an empty file, a non-zero size must be
> specified by the option 'size'.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  backends/hostmem-file.c | 10 ++++++----
>  exec.c                  |  8 ++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 42efb2f..f94d2f7 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -40,10 +40,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>  {
>      HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>  
> -    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;
> @@ -62,6 +58,12 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>          g_free(path);
>      }
>  #endif
> +    if (!errp && !backend->size) {

This condition is always false because non-NULL errp is always
provided by the only caller (host_memory_backend_memory_complete()).

To simplify error checking, I suggest moving the error path to a
label at the end of the function, e.g.:

static void file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
{
    Error *local_err = NULL;
    /* ... */
    memory_region_init_ram_from_file(..., &local_err);
    if (local_err) {
        goto out;
    }
    /* ... */
    if (!backend->size) {
        backend->size = memory_region_size(&backend->mr);
    }
    /* ... */
out:
    error_propagate(errp, local_err);
}

> +        backend->size = memory_region_size(&backend->mr);
> +        if (!backend->size) {
> +            error_setg(errp, "can't create backend with size 0");
> +        }
> +    }
>  }
>  
>  static char *get_mem_path(Object *o, Error **errp)
> diff --git a/exec.c b/exec.c
> index 95983c9..91adc62 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1274,6 +1274,14 @@ static void *file_ram_alloc(RAMBlock *block,
>          goto error;
>      }
>  
> +    if (memory) {
> +        memory = memory ?: file_size;

This doesn't make sense to me. You already checked if memory is
zero above, and now you are checking if it's zero again.
file_size is never going to be used here.

> +        memory_region_set_size(block->mr, memory);
> +        memory = HOST_PAGE_ALIGN(memory);
> +        block->used_length = memory;
> +        block->max_length = memory;

This is fragile: it duplicates the logic that initializes
used_length and max_length in qemu_ram_alloc_*().

Maybe it's better to keep the file-size-probing magic inside
hostmem-file.c, and always give a non-zero size to
memory_region_init_ram_from_file().

> +    }
> +
>      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",
> -- 
> 2.10.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional

[Haozhong Zhang]

On 10/25/16 17:50 -0200, Eduardo Habkost wrote:
>On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote:
>> If 'size' option of hostmem-file is not given, QEMU will use the file
>> size of 'mem-path' instead. For an empty file, a non-zero size must be
>> specified by the option 'size'.
>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> ---
>>  backends/hostmem-file.c | 10 ++++++----
>>  exec.c                  |  8 ++++++++
>>  2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>> index 42efb2f..f94d2f7 100644
>> --- a/backends/hostmem-file.c
>> +++ b/backends/hostmem-file.c
>> @@ -40,10 +40,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>  {
>>      HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
>>
>> -    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;
>> @@ -62,6 +58,12 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>          g_free(path);
>>      }
>>  #endif
>> +    if (!errp && !backend->size) {
>
>This condition is always false because non-NULL errp is always
>provided by the only caller (host_memory_backend_memory_complete()).
>

Oops, I meant !*errp. Anyway, I'll change to the way you suggested below.

>To simplify error checking, I suggest moving the error path to a
>label at the end of the function, e.g.:
>
>static void file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>{
>    Error *local_err = NULL;
>    /* ... */
>    memory_region_init_ram_from_file(..., &local_err);
>    if (local_err) {
>        goto out;
>    }
>    /* ... */
>    if (!backend->size) {
>        backend->size = memory_region_size(&backend->mr);
>    }
>    /* ... */
>out:
>    error_propagate(errp, local_err);
>}
>
>> +        backend->size = memory_region_size(&backend->mr);
>> +        if (!backend->size) {
>> +            error_setg(errp, "can't create backend with size 0");
>> +        }
>> +    }
>>  }
>>
>>  static char *get_mem_path(Object *o, Error **errp)
>> diff --git a/exec.c b/exec.c
>> index 95983c9..91adc62 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1274,6 +1274,14 @@ static void *file_ram_alloc(RAMBlock *block,
>>          goto error;
>>      }
>>
>> +    if (memory) {
>> +        memory = memory ?: file_size;
>
>This doesn't make sense to me. You already checked if memory is
>zero above, and now you are checking if it's zero again.
>file_size is never going to be used here.
>
>> +        memory_region_set_size(block->mr, memory);
>> +        memory = HOST_PAGE_ALIGN(memory);
>> +        block->used_length = memory;
>> +        block->max_length = memory;
>
>This is fragile: it duplicates the logic that initializes
>used_length and max_length in qemu_ram_alloc_*().
>
>Maybe it's better to keep the file-size-probing magic inside
>hostmem-file.c, and always give a non-zero size to
>memory_region_init_ram_from_file().
>

Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file().

Thanks,
Haozhong

>> +    }
>> +
>>      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",
>> --
>> 2.10.1
>>
>
>-- 
>Eduardo

Re: [Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional

[Haozhong Zhang]

On 10/26/16 13:56 +0800, Haozhong Zhang wrote:
>On 10/25/16 17:50 -0200, Eduardo Habkost wrote:
>>On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote:
>>>+    if (memory) {
>>>+        memory = memory ?: file_size;
>>
>>This doesn't make sense to me. You already checked if memory is
>>zero above, and now you are checking if it's zero again.
>>file_size is never going to be used here.
>>
>>>+        memory_region_set_size(block->mr, memory);
>>>+        memory = HOST_PAGE_ALIGN(memory);
>>>+        block->used_length = memory;
>>>+        block->max_length = memory;
>>
>>This is fragile: it duplicates the logic that initializes
>>used_length and max_length in qemu_ram_alloc_*().
>>
>>Maybe it's better to keep the file-size-probing magic inside
>>hostmem-file.c, and always give a non-zero size to
>>memory_region_init_ram_from_file().
>>
>
>Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file().
>

Maybe no. Two reasons:
1) Patch 1 has some code related to file size and I'd like to put all
   logic related to file size in the same function.
2) file_ram_alloc() has the logic to open/create/close the backend file
   and handle errors in this process, which also needs to move to
   qemu_ram_alloc_from_file() if file-size-probing is moved.

Instead, I'd
1) keep all logic related to file size in file_ram_alloc()

2) let file_ram_alloc() return the value of 'memory', e.g.
     static void *file_ram_alloc(RAMBlock *block,
    -                            ram_addr_t *memory,
    +                            ram_addr_t memory,
                                 const char *path,
                                 Error **errp)
     {
         ...
         // other usages of 'memory' are changed as well
    -    memory = ROUND_UP(*memory, block->page_size);   
    +    *memory = ROUND_UP(*memory, block->page_size);
         ...
     }
     
3) in qemu_ram_alloc_from_file(), move setting
   block->used_length/max_length after calling file_ram_alloc(),
   e.g.
     RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
                                        bool share, const char *mem_path,
                                        Error **errp)
     {
         ...
         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;
         ...
     }

Re: [Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional

[Eduardo Habkost]

On Wed, Oct 26, 2016 at 03:49:29PM +0800, Haozhong Zhang wrote:
> On 10/26/16 13:56 +0800, Haozhong Zhang wrote:
> > On 10/25/16 17:50 -0200, Eduardo Habkost wrote:
> > > On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote:
> > > > +    if (memory) {
> > > > +        memory = memory ?: file_size;
> > > 
> > > This doesn't make sense to me. You already checked if memory is
> > > zero above, and now you are checking if it's zero again.
> > > file_size is never going to be used here.
> > > 
> > > > +        memory_region_set_size(block->mr, memory);
> > > > +        memory = HOST_PAGE_ALIGN(memory);
> > > > +        block->used_length = memory;
> > > > +        block->max_length = memory;
> > > 
> > > This is fragile: it duplicates the logic that initializes
> > > used_length and max_length in qemu_ram_alloc_*().
> > > 
> > > Maybe it's better to keep the file-size-probing magic inside
> > > hostmem-file.c, and always give a non-zero size to
> > > memory_region_init_ram_from_file().
> > > 
> > 
> > Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file().
> > 
> 
> Maybe no. Two reasons:
> 1) Patch 1 has some code related to file size and I'd like to put all
>   logic related to file size in the same function.
> 2) file_ram_alloc() has the logic to open/create/close the backend file
>   and handle errors in this process, which also needs to move to
>   qemu_ram_alloc_from_file() if file-size-probing is moved.

I'd argue to move the whole file creation/opening logic to
hostmem-file.c. But it wouldn't be a small amount of work, so
your points make sense.

The plan below could work, but I would like it to get feedback
from the Memory API maintainer (Paolo).

> 
> Instead, I'd
> 1) keep all logic related to file size in file_ram_alloc()
> 
> 2) let file_ram_alloc() return the value of 'memory', e.g.
>     static void *file_ram_alloc(RAMBlock *block,
>    -                            ram_addr_t *memory,
>    +                            ram_addr_t memory,
>                                 const char *path,
>                                 Error **errp)
>     {
>         ...
>         // other usages of 'memory' are changed as well
>    -    memory = ROUND_UP(*memory, block->page_size);      +    *memory =
> ROUND_UP(*memory, block->page_size);
>         ...
>     }
> 3) in qemu_ram_alloc_from_file(), move setting
>   block->used_length/max_length after calling file_ram_alloc(),
>   e.g.
>     RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>                                        bool share, const char *mem_path,
>                                        Error **errp)
>     {
>         ...
>         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;
>         ...
>     }
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] hostmem-file: allow option 'size' optional

[Paolo Bonzini]

On 26/10/2016 16:17, Eduardo Habkost wrote:
> On Wed, Oct 26, 2016 at 03:49:29PM +0800, Haozhong Zhang wrote:
>> On 10/26/16 13:56 +0800, Haozhong Zhang wrote:
>>> On 10/25/16 17:50 -0200, Eduardo Habkost wrote:
>>>> On Mon, Oct 24, 2016 at 05:21:51PM +0800, Haozhong Zhang wrote:
>>>>> +    if (memory) {
>>>>> +        memory = memory ?: file_size;
>>>>
>>>> This doesn't make sense to me. You already checked if memory is
>>>> zero above, and now you are checking if it's zero again.
>>>> file_size is never going to be used here.
>>>>
>>>>> +        memory_region_set_size(block->mr, memory);
>>>>> +        memory = HOST_PAGE_ALIGN(memory);
>>>>> +        block->used_length = memory;
>>>>> +        block->max_length = memory;
>>>>
>>>> This is fragile: it duplicates the logic that initializes
>>>> used_length and max_length in qemu_ram_alloc_*().
>>>>
>>>> Maybe it's better to keep the file-size-probing magic inside
>>>> hostmem-file.c, and always give a non-zero size to
>>>> memory_region_init_ram_from_file().
>>>>
>>>
>>> Yes, I can move the logic in above if-statement to qemu_ram_alloc_from_file().
>>>
>>
>> Maybe no. Two reasons:
>> 1) Patch 1 has some code related to file size and I'd like to put all
>>   logic related to file size in the same function.
>> 2) file_ram_alloc() has the logic to open/create/close the backend file
>>   and handle errors in this process, which also needs to move to
>>   qemu_ram_alloc_from_file() if file-size-probing is moved.
> 
> I'd argue to move the whole file creation/opening logic to
> hostmem-file.c. But it wouldn't be a small amount of work, so
> your points make sense.
> 
> The plan below could work, but I would like it to get feedback
> from the Memory API maintainer (Paolo).

Yes, it makes sense.

Paolo

>>
>> Instead, I'd
>> 1) keep all logic related to file size in file_ram_alloc()
>>
>> 2) let file_ram_alloc() return the value of 'memory', e.g.
>>     static void *file_ram_alloc(RAMBlock *block,
>>    -                            ram_addr_t *memory,
>>    +                            ram_addr_t memory,
>>                                 const char *path,
>>                                 Error **errp)
>>     {
>>         ...
>>         // other usages of 'memory' are changed as well
>>    -    memory = ROUND_UP(*memory, block->page_size);      +    *memory =
>> ROUND_UP(*memory, block->page_size);
>>         ...
>>     }
>> 3) in qemu_ram_alloc_from_file(), move setting
>>   block->used_length/max_length after calling file_ram_alloc(),
>>   e.g.
>>     RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>>                                        bool share, const char *mem_path,
>>                                        Error **errp)
>>     {
>>         ...
>>         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;
>>         ...
>>     }
>>
>