On Thu, 21 Jul 2016 10:36:40 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Igor Mammedov <imamm...@redhat.com> writes: [...] > > @@ -286,8 +291,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, > > Error **errp) > > if (bc->alloc) { > > bc->alloc(backend, &local_err); > > if (local_err) { > > - error_propagate(errp, local_err); > > - return; > > + goto out; > > I'd leave the tail merging optimization to the compiler, i.e. don't > touch the code here, and ... > > > } > > > > ptr = memory_region_get_ram_ptr(&backend->mr); > > @@ -343,9 +347,15 @@ host_memory_backend_memory_complete(UserCreatable *uc, > > Error **errp) > > * specified NUMA policy in place. > > */ > > if (backend->prealloc) { > > - os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz); > > + os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz, > > + &local_err); > > + if (local_err) { > > + goto out; > > ... have the obvious error_propagate() + return here. Matter of taste, > between you and the maintainer. Except there is none. Inexcusable for > a file created in 2014. Suggest you appoint yourself. > > > + } I might if Eduardo declines or I could be and additional one. wrt consolidating error_propagate(), it's what we were doing in target-i386/cpu.c any time we touched similar code pathes to reduce code duplication. > > } > > } > > +out: > > + error_propagate(errp, local_err); > > } [...] > > @@ -352,15 +352,14 @@ void os_mem_prealloc(int fd, char *area, size_t > > memory) > > for (i = 0; i < numpages; i++) { > > memset(area + (hpagesize * i), 0, 1); > > } > > + } > > > > - ret = sigaction(SIGBUS, &oldact, NULL); > > - if (ret) { > > - perror("os_mem_prealloc: failed to reinstall signal handler"); > > - exit(1); > > - } > > - > > - pthread_sigmask(SIG_SETMASK, &oldset, NULL); > > + ret = sigaction(SIGBUS, &oldact, NULL); > > + if (ret) { > > + perror("os_mem_prealloc: failed to reinstall signal handler"); > > + exit(1); > > I guess you're keeping the exit() here, because you can't recover > cleanly from this error. I should never happen anyway. Worth a > comment, I think. I didn't added comment because it's obvious that it's not possible to recover and also because it's just the same old code that haven't had a comment to begin with (probably also due to its obviousness) > > } > > + pthread_sigmask(SIG_SETMASK, &oldset, NULL); > > } > > > > > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > > index 6debc2b..4c1dcf1 100644 > > --- a/util/oslib-win32.c > > +++ b/util/oslib-win32.c > > @@ -539,7 +539,7 @@ int getpagesize(void) > > return system_info.dwPageSize; > > } > > > > -void os_mem_prealloc(int fd, char *area, size_t memory) > > +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) > > { > > int i; > > size_t pagesize = getpagesize(); > > Reviewed-by: Markus Armbruster <arm...@redhat.com> Thanks