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

[Igor Mammedov]

On Fri, 22 Jul 2016 18:19:44 +0200
Markus Armbruster <address@hidden> wrote:

> Igor Mammedov <address@hidden> writes:
> 
> > On Thu, 21 Jul 2016 10:36:40 +0200
> > Markus Armbruster <address@hidden> wrote:
> >  
> >> Igor Mammedov <address@hidden> writes:  
> [...]
> >> > @@ -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)  
> 
> It's obvious enough once you read closely enough to see what the failing
> code is doing.
> 
> The old code uses perror() + exit() consistently.  The new code mixes
> Error with them, which is an anti-pattern.  That's okay, the exit() is
> perfectly fine here.  The issue for me is that the anti-pattern triggers
> the WTF-detector at a glance, but then I have to read closely to see #1
> it's intentional, and #2 it's okay.  That wasn't the case before.
> 
> [...]

I'll post a patch to add comment on top of this one.