On Thu, May 10, 2012 at 6:04 AM, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
> + case SIG_BLOCK:
> + sigaddsetmask(&new_blocked, new_set);
> + break;
> + case SIG_UNBLOCK:
> + sigdelsetmask(&new_blocked, new_set);
> + break;
Ok, I think SIG_[UN]BLOCK are now clearly right. However:
> + case SIG_SETMASK:
> + new_blocked.sig[0] &=
> + ~((old_sigset_t)(compat_old_sigset_t)-1);
> + new_blocked.sig[0] |= new_set;
> + break;
I don't think this is clear.
The semantics for the *native* SIG_SETMASK has been to only change the
lower word of the sigset_t.
And that was actually defined in terms of "compat_sigset_word", not
"compat_old_sigset_t".
Now, they are both generally the same, and so I think your code does
the right thing, but I have to say that I really had to look closely
to make sure that yes, your code was right.
Anyway, my *gut* feel is that it would be much clearer to write the above as
compat_sigset_word x = new_set;
memcpy(new_blocked.sig, &x, sizeof(x));
together with a comment to the effect that sigprocmask(SIG_SETMASK..)
only changes the first word of the structure.
That said, I think your patch does look technically correct, so maybe
it's just me who thinks it is very non-obvious and hard to read.
The memcpy approach will also generate better code. This is the "mask-and-set":
movabsq $-4294967296, %rax #, tmp89
andq -32(%rbp), %rax # new_blocked.sig, tmp89
orq %rdx, %rax # new_set, tmp89
movq %rax, -32(%rbp) # tmp89, new_blocked.sig
and this is the memcpy:
movl %edx, -32(%rbp) # new_set,
ie it is done as a simple 32-bit store.
I think I'll just edit your patch directly, no need to send me a new version.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
