Re: [Qemu-devel] [PATCH for-2.7] fix qemu exit on memory hotplug when allocation fails at prealloc time

[Igor Mammedov]

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