[Qemu-devel] [PATCH 0/2] Style and bug on linux-user/signal.c

[Qemu-devel] [PATCH 0/2] Style and bug on linux-user/signal.c

[Alex Barcelo]

I tried to send a trivial patch and a v2, but I did it horribly wrong
and seems that have not yet been updated. I thought that maybe
it was a good moment to do things "betterly".

The bug is really a typo, because everywhere there is the same
0 comparison. But each comparison is different one from another, so
the style is quite messed up and the typo becomes non-trivial to
understand.

The first patch is a style patch which makes everything check the
conditions the same way. The second patch corrects a "== 0" that
was missing on an if condition, to be like every other condition.

I sent two "[TRIVIAL]" to the lists that are irrelevant with this
patch. They are:
"sas_ss_flags bug for powerpc"
(v2) "Bad zero comparison for sas_ss_flags on powerpc"

Alex Barcelo (2):
  linux-user: Homogeneity on sas_ss_flags checks (signal)
  linux-user: Bug on a zero comparation with sas_ss_flags

 linux-user/signal.c |   46 ++++++++++++++++++++++++++--------------------
 1 files changed, 26 insertions(+), 20 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH 1/2] linux-user: Homogeneity on sas_ss_flags checks (signal)

[Alex Barcelo]

Each architecture does the same comparation, but it is hard
(at least was hard for me) to see, because of the fancy way
of doing a simple 0 comparation.

This patch simply tries to assure signal.c code coherence.

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 linux-user/signal.c |   44 +++++++++++++++++++++++++-------------------
 1 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index fca51e2..d1a2671 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -804,19 +804,21 @@ get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t frame_size)
 	/* Default to using normal stack */
 	esp = env->regs[R_ESP];
 	/* This is the X/Open sanctioned signal stack switching.  */
-        if (ka->sa_flags & TARGET_SA_ONSTACK) {
-            if (sas_ss_flags(esp) == 0)
-                esp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
+    if (ka->sa_flags & TARGET_SA_ONSTACK) {
+        if (sas_ss_flags(esp) == 0) {
+            esp = (target_sigaltstack_used.ss_sp
+                   + target_sigaltstack_used.ss_size);
         }
-
-	/* This is the legacy signal stack switching. */
-	else
+    } else {
+        /* This is the legacy signal stack switching. */
         if ((env->segs[R_SS].selector & 0xffff) != __USER_DS &&
             !(ka->sa_flags & TARGET_SA_RESTORER) &&
             ka->sa_restorer) {
             esp = (unsigned long) ka->sa_restorer;
-	}
-        return (esp - frame_size) & -8ul;
+        }
+    }
+
+    return (esp - frame_size) & -8ul;
 }
 
 /* compare linux/arch/i386/kernel/signal.c:setup_frame() */
@@ -1248,8 +1250,10 @@ get_sigframe(struct target_sigaction *ka, CPUARMState *regs, int framesize)
 	/*
 	 * This is the X/Open sanctioned signal stack switching.
 	 */
-	if ((ka->sa_flags & TARGET_SA_ONSTACK) && !sas_ss_flags(sp))
-            sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
+    if ((ka->sa_flags & TARGET_SA_ONSTACK) &&
+        (sas_ss_flags(sp) == 0)) {
+        sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
+    }
 	/*
 	 * ATPCS B01 mandates 8-byte alignment
 	 */
@@ -2710,7 +2714,8 @@ get_sigframe(struct target_sigaction *ka, CPUMIPSState *regs, size_t frame_size)
     sp -= 32;
 
     /* This is the X/Open sanctioned signal stack switching.  */
-    if ((ka->sa_flags & TARGET_SA_ONSTACK) && (sas_ss_flags (sp) == 0)) {
+    if ((ka->sa_flags & TARGET_SA_ONSTACK) &&
+        (sas_ss_flags(sp) == 0)) {
         sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
     }
 
@@ -2969,7 +2974,8 @@ struct target_rt_sigframe
 static abi_ulong get_sigframe(struct target_sigaction *ka,
                          unsigned long sp, size_t frame_size)
 {
-    if ((ka->sa_flags & TARGET_SA_ONSTACK) && (sas_ss_flags(sp) == 0)) {
+    if ((ka->sa_flags & TARGET_SA_ONSTACK) &&
+        (sas_ss_flags(sp) == 0)) {
         sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
     }
 
@@ -3698,11 +3704,9 @@ get_sigframe(struct target_sigaction *ka, CPUS390XState *env, size_t frame_size)
     sp = env->regs[15];
 
     /* This is the X/Open sanctioned signal stack switching.  */
-    if (ka->sa_flags & TARGET_SA_ONSTACK) {
-        if (!sas_ss_flags(sp)) {
-            sp = target_sigaltstack_used.ss_sp +
-                 target_sigaltstack_used.ss_size;
-        }
+    if ((ka->sa_flags & TARGET_SA_ONSTACK) &&
+        (sas_ss_flags(sp) == 0) {
+        sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
     }
 
     /* This is the legacy signal stack switching. */
@@ -4668,7 +4672,8 @@ get_sigframe(struct target_sigaction *ka, CPUM68KState *regs,
     sp = regs->aregs[7];
 
     /* This is the X/Open sanctioned signal stack switching.  */
-    if ((ka->sa_flags & TARGET_SA_ONSTACK) && (sas_ss_flags (sp) == 0)) {
+    if ((ka->sa_flags & TARGET_SA_ONSTACK) &&
+        (sas_ss_flags(sp) == 0)) {
         sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
     }
 
@@ -5046,7 +5051,8 @@ static inline abi_ulong get_sigframe(struct target_sigaction *sa,
     abi_ulong sp = env->ir[IR_SP];
 
     /* This is the X/Open sanctioned signal stack switching.  */
-    if ((sa->sa_flags & TARGET_SA_ONSTACK) != 0 && !sas_ss_flags(sp)) {
+    if ((sa->sa_flags & TARGET_SA_ONSTACK) &&
+        (sas_ss_flags(sp) == 0)) {
         sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
     }
     return (sp - framesize) & -32;
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/2] linux-user: Bug on a zero comparation with sas_ss_flags

[Alex Barcelo]

All sas_ss_flags' 0 check is the same across every architecture.
But in PPC there was a bug and it was checked the other way round.

It seems a typo and is not architecture dependant, is a POSIX
standard and the QEMU way of checking it.

Now the signal.c and its sas_ss_flags check should be coherent and
correct across archs.

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 linux-user/signal.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index d1a2671..d159ada 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -4122,7 +4122,7 @@ static target_ulong get_sigframe(struct target_sigaction *ka,
     oldsp = env->gpr[1];
 
     if ((ka->sa_flags & TARGET_SA_ONSTACK) &&
-        (sas_ss_flags(oldsp))) {
+        (sas_ss_flags(oldsp) == 0)) {
         oldsp = (target_sigaltstack_used.ss_sp
                  + target_sigaltstack_used.ss_size);
     }
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 0/2] Style and bug on linux-user/signal.c

[Stefan Hajnoczi]

On Thu, Mar 15, 2012 at 8:52 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> I tried to send a trivial patch and a v2, but I did it horribly wrong
> and seems that have not yet been updated. I thought that maybe
> it was a good moment to do things "betterly".

Alex Graf mentioned he'd apply it to ppc-next, I have CCed him but he
may have limited connectivity because I think he's away at the moment.
 Perhaps Riku can review and merge it.

Stefan

Re: [Qemu-devel] [PATCH 0/2] Style and bug on linux-user/signal.c

[Alexander Graf]

On 15.03.2012, at 12:41, Stefan Hajnoczi wrote:

> On Thu, Mar 15, 2012 at 8:52 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>> I tried to send a trivial patch and a v2, but I did it horribly wrong
>> and seems that have not yet been updated. I thought that maybe
>> it was a good moment to do things "betterly".
> 
> Alex Graf mentioned he'd apply it to ppc-next, I have CCed him but he
> may have limited connectivity because I think he's away at the moment.
> Perhaps Riku can review and merge it.

No, I'm still around until Monday and today is pull request day! Hooray! So after long times of anxious waiting, we will finally have lots of fun ppc stuff in the tree again ;).


Alex

Re: [Qemu-devel] [PATCH 0/2] Style and bug on linux-user/signal.c

[Alex Barcelo]

Thanks and sorry! I forgot to add a CC when doing the git send-email.

And I thought that I saw some pulls to PPC, so I assumed that the
patch was incomplete (I didn't prepare it very well), and thought that
better to do a style patch next to it. And comment it better.

On Thu, Mar 15, 2012 at 12:47, Alexander Graf <agraf@suse.de> wrote:
>
> On 15.03.2012, at 12:41, Stefan Hajnoczi wrote:
>
>> On Thu, Mar 15, 2012 at 8:52 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>>> I tried to send a trivial patch and a v2, but I did it horribly wrong
>>> and seems that have not yet been updated. I thought that maybe
>>> it was a good moment to do things "betterly".
>>
>> Alex Graf mentioned he'd apply it to ppc-next, I have CCed him but he
>> may have limited connectivity because I think he's away at the moment.
>> Perhaps Riku can review and merge it.
>
> No, I'm still around until Monday and today is pull request day! Hooray! So after long times of anxious waiting, we will finally have lots of fun ppc stuff in the tree again ;).
>
>
> Alex
>