[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.7] fix qemu exit on memory hotplug when al
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH for-2.7] fix qemu exit on memory hotplug when allocation fails at prealloc time |
Date: |
Thu, 21 Jul 2016 13:39:58 +0200 |
On Thu, 21 Jul 2016 10:36:40 +0200
Markus Armbruster <address@hidden> wrote:
> Igor Mammedov <address@hidden> 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 <address@hidden>
Thanks
[Qemu-devel] [PATCH] fixup! fix qemu exit on memory hotplug when allocation fails at prealloc time, Igor Mammedov, 2016/07/25