On Fri, 22 Jul 2016 18:19:44 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Igor Mammedov <imamm...@redhat.com> writes:
>
> > On Thu, 21 Jul 2016 10:36:40 +0200
> > Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> Igor Mammedov <imamm...@redhat.com> 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.
