Messages in this thread | | | Date | Fri, 29 Feb 2008 08:26:14 -0800 (PST) | From | Linus Torvalds <> | Subject | Re: [PATCH] x86_64 ia32 syscall restart fix |
| |
On Fri, 29 Feb 2008, Ingo Molnar wrote: > > * Roland McGrath <roland@redhat.com> wrote: > > > The code to restart syscalls after signals depends on checking for a > > negative orig_ax, and for particular negative -ERESTART* values in ax. > > These fields are 64 bits and for a 32-bit task they get zero-extended. > > The syscall restart behavior is lost, a regression from a native > > 32-bit kernel and from 64-bit tasks' behavior. This patch fixes the > > problem by doing sign-extension where it matters. For orig_ax, the > > only time the value should be -1 but winds up as 0x0ffffffff is via a > > 32-bit ptrace call. So the patch changes ptrace to sign-extend the > > 32-bit orig_eax value when it's stored; it doesn't change the checks > > on orig_ax, though it uses the new current_syscall() inline to better > > document the subtle importance of the used of signedness there. The > > ax value is stored a lot of ways and it seems hard to get them all > > sign-extended at their origins. So for that, we use the > > current_syscall_ret() to sign-extend it only for 32-bit tasks at the > > time of the -ERESTART* comparisons. > > thanks, applied.
Btw, can we please try to keep commit log messages readable?
The above "blob of text" could/should have more structure than being just one big block, and could have been structured as a few shorter paragraphs to make it easier to read: (1) problem description (2) patch description and (3) explanation of why patch was done it was done.
I don't know about you guys, but I read a *lot* of emails (and commit messages), and I hate seeing big blobs of text without structure. Give it a few breaks to make it easier to read, like just making new paragraphs, ie something like:
> The code to restart syscalls after signals depends on checking for a > negative orig_ax, and for particular negative -ERESTART* values in ax. > These fields are 64 bits and for a 32-bit task they get zero-extended. > The syscall restart behavior is lost, a regression from a native 32-bit > kernel and from 64-bit tasks' behavior. > > This patch fixes the problem by doing sign-extension where it matters. > For orig_ax, the only time the value should be -1 but winds up as > 0x0ffffffff is via a 32-bit ptrace call. So the patch changes ptrace to > sign-extend the 32-bit orig_eax value when it's stored; it doesn't > change the checks on orig_ax, though it uses the new current_syscall() > inline to better document the subtle importance of the used of > signedness there. > > The ax value is stored a lot of ways and it seems hard to get them all > sign-extended at their origins. So for that, we use the > current_syscall_ret() to sign-extend it only for 32-bit tasks at the > time of the -ERESTART* comparisons.
and now you have a bit of a breather space and some visual cues for whare you are in the text.
Yeah, maybe it's just me, but I like my whitespace. Ihaveareallyhardtime readingtextthatdoesn'thavethepropermarkersforwhereconceptsstartandbegin, andthatreallydoesincludetheverticalwhitespacetoo.
Now, the only reason I mention this is that normally I would probably just have fixed this up myself without even a comment (because it's such a tiny detail that it's not not worth one), but when Ingo merges it I'll now get it through git and it will be fixed.
Linus "yeah, I can be anal" Torvalds
| |