| Message ID | bca18866621abd15155f50f87b98a27c3deeb4b1.1407302379.git.hutao@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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 <hutao@cn.fujitsu.com> 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 <hutao@cn.fujitsu.com> > --- > 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 > >
On Wed, Aug 06, 2014 at 10:49:23PM +1000, Peter Crosthwaite wrote: > You subject line is excessively long. How about just "improve RAM file > error handling" and elaborate in a commit msg para? After splitting both patches are improving of RAM file error handling. I'll reword the subjects and drop this one. > > On Wed, Aug 6, 2014 at 3:36 PM, Hu Tao <hutao@cn.fujitsu.com> 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 <hutao@cn.fujitsu.com> > > --- > > 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). changed to: failed to get page size of file > > > 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. Okay. > > > goto error; > > } > > > > if (memory < hpagesize) { > > - return NULL; > > + error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be euqal to " > > "equal" thanks! > > 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 > > > >
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); 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) { goto error; } if (memory < hpagesize) { - return NULL; + error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be euqal 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); + if (area && area != MAP_FAILED) { + munmap(area, memory); } return NULL; }
This patch fixes two problems of memory-backend-file: 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 Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- exec.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)