Re: [Qemu-devel] [PATCH v5 6/6] exec: improve error handling and reporting in file_ram_alloc() and gethugepagesize()

[Peter Crosthwaite]

You subject line is excessively long. How about just "improve RAM file
error handling" and elaborate in a commit msg para?

On Wed, Aug 6, 2014 at 3:36 PM, Hu Tao <address@hidden> wrote:
> This patch fixes two problems of memory-backend-file:
>

It looks like two self contained changes. Any reason to not split?

> 1. If user adds a memory-backend-file object using object_add command,
>    specifying a non-existing directory for property mem-path, qemu
>    will core dump with message:
>
>      /nonexistingdir: No such file or directory
>      Bad ram offset fffffffffffff000
>      Aborted (core dumped)
>
>    with this patch, qemu reports error message like:
>
>      qemu-system-x86_64: -object 
> memory-backend-file,mem-path=/nonexistingdir,id=mem-file0,size=128M:
>      failed to stat file /nonexistingdir: No such file or directory
>
> 2. If user adds a memory-backend-file object using object_add command,
>    specifying a size that is less than huge page size, qemu
>    will core dump with message:
>
>      Bad ram offset fffffffffffff000
>      Aborted (core dumped)
>
>    with this patch, qemu reports error message like:
>
>      qemu-system-x86_64: -object 
> memory-backend-file,mem-path=/hugepages,id=mem-file0,size=1M: memory
>      size 0x100000 should be euqal or larger than huge page size 0x200000

"equal".

>
> Signed-off-by: Hu Tao <address@hidden>
> ---
>  exec.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 7e60a44..6512820 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -996,7 +996,7 @@ void qemu_mutex_unlock_ramlist(void)
>

>  #define HUGETLBFS_MAGIC       0x958458f6
>
> -static long gethugepagesize(const char *path)
> +static long gethugepagesize(const char *path, Error **errp)
>  {
>      struct statfs fs;
>      int ret;
> @@ -1006,7 +1006,7 @@ static long gethugepagesize(const char *path)
>      } while (ret != 0 && errno == EINTR);
>
>      if (ret != 0) {
> -        perror(path);
> +        error_setg_errno(errp, errno, "failed to get size of file %s", path);

I think your error message is imprecise. It's not the file size you
are trying to get its the page size for that file (or its underlying
file system I think).

>          return 0;
>      }
>
> @@ -1024,17 +1024,20 @@ static void *file_ram_alloc(RAMBlock *block,
>      char *filename;
>      char *sanitized_name;
>      char *c;
> -    void *area;
> +    void *area = NULL;
>      int fd;
> -    unsigned long hpagesize;
> +    uint64_t hpagesize;
>
> -    hpagesize = gethugepagesize(path);
> -    if (!hpagesize) {
> +    hpagesize = gethugepagesize(path, errp);
> +    if (errp && *errp) {

More flow control dependent on non NULL errp. I think you want a
local_err for safety here.

>          goto error;
>      }
>
>      if (memory < hpagesize) {
> -        return NULL;
> +        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be euqal to "

"equal"

Regards,
Peter

> +                   "or larger than huge page size 0x%" PRIx64,
> +                   memory, hpagesize);
> +        goto error;
>      }
>
>      if (kvm_enabled() && !kvm_has_sync_mmu()) {
> @@ -1094,8 +1097,8 @@ static void *file_ram_alloc(RAMBlock *block,
>      return area;
>
>  error:
> -    if (mem_prealloc) {
> -        exit(1);
> +    if (area && area != MAP_FAILED) {
> +        munmap(area, memory);
>      }
>      return NULL;
>  }
> --
> 1.9.3
>
>