[v6,6/7] exec: report error when memory < hpagesize

Report error when memory < hpagesize in file_ram_alloc() so callers can
handle the error.

This patch fix a problem that 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 must be equal to or larger than huge page size 0x200000

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 exec.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

On Thu, Aug 7, 2014 at 7:10 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> Report error when memory < hpagesize in file_ram_alloc() so callers can

"an error"

> handle the error.
>
> This patch fix a problem that if user adds a memory-backend-file object

Long sentence. I would drop the "This patch fixes a problem that"

> 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)
>

Then say here "This patch fixes the problem."

> 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 must be equal to or larger than huge page size 0x200000
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  exec.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index accba00..50cd510 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1024,9 +1024,9 @@ 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) {
> @@ -1034,7 +1034,10 @@ static void *file_ram_alloc(RAMBlock *block,
>      }
>
>      if (memory < hpagesize) {
> -        return NULL;
> +        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
> +                   "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);

So I get the movitation behind getting rid of the core dump and
abort(). But this seems like a different change. You are demoting an
explicit fatal error (which looks to me to be unhelpfully silent) to a
propagating error. What's the reasoning? Is there any awareness of the
must-exit on the mem_prealloc failure case in the higher levels?

Regards,
Peter

> +    if (area && area != MAP_FAILED) {
> +        munmap(area, memory);
>      }
>      return NULL;
>  }
> --
> 1.9.3
>
>

On Thu, Aug 07, 2014 at 09:45:04PM +1000, Peter Crosthwaite wrote:
> On Thu, Aug 7, 2014 at 7:10 PM, Hu Tao <hutao@cn.fujitsu.com> wrote:
> > Report error when memory < hpagesize in file_ram_alloc() so callers can
> 
> "an error"
> 
> > handle the error.
> >
> > This patch fix a problem that if user adds a memory-backend-file object
> 
> Long sentence. I would drop the "This patch fixes a problem that"
> 
> > 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)
> >
> 
> Then say here "This patch fixes the problem."

I'll reword if respin.

> 
> > 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 must be equal to or larger than huge page size 0x200000
> >
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  exec.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index accba00..50cd510 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1024,9 +1024,9 @@ 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) {
> > @@ -1034,7 +1034,10 @@ static void *file_ram_alloc(RAMBlock *block,
> >      }
> >
> >      if (memory < hpagesize) {
> > -        return NULL;
> > +        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
> > +                   "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);
> 
> So I get the movitation behind getting rid of the core dump and
> abort(). But this seems like a different change. You are demoting an
> explicit fatal error (which looks to me to be unhelpfully silent) to a
> propagating error. What's the reasoning? Is there any awareness of the
> must-exit on the mem_prealloc failure case in the higher levels?

Indeed this change is irrelevant to this patch, but relevant(not quite)
to patch 5.  The change fixes a similar problem described in patch 5 but
at the time preallocating failed.

Steps to reproduce this problem are similar to the ones in patch 5,
except that adds -mem-prealloc to qemu command line.

Regards,
Hu

> 
> Regards,
> Peter
> 
> > +    if (area && area != MAP_FAILED) {
> > +        munmap(area, memory);
> >      }
> >      return NULL;
> >  }
> > --
> > 1.9.3
> >
> >