Patchwork sparc: make proces_ver_nack a bit more readable

login
register
mail settings
Submitter Steven Rostedt
Date Jan. 5, 2009, 7:42 p.m.
Message ID <alpine.DEB.1.10.0901051437490.27085@gandalf.stny.rr.com>
Download mbox | patch
Permalink /patch/16681/
State Accepted
Delegated to: David Miller
Headers show

Comments

Steven Rostedt - Jan. 5, 2009, 7:42 p.m.
Impact: clean up

The code in process_ver_nack is a little obfuscated. This change
makes it a bit more readable by humans. It removes the complex
if statement and replaces it with a cleaner flow of control.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>



--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt - Jan. 5, 2009, 7:46 p.m.
On Mon, 5 Jan 2009, Steven Rostedt wrote:

> 
> Impact: clean up
> 
> The code in process_ver_nack is a little obfuscated. This change
> makes it a bit more readable by humans. It removes the complex
> if statement and replaces it with a cleaner flow of control.

Note, I do not have a sparc compiler at my disposal, so I was not
able to compile (or boot) test this change.

Here's the original code:

static int process_ver_nack(struct ldc_channel *lp, struct ldc_version 
*vp)
{
        struct ldc_version *vap;

        if ((vp->major == 0 && vp->minor == 0) ||
            !(vap = find_by_major(vp->major))) {
                return ldc_abort(lp);
        } else {
                struct ldc_packet *p;
                unsigned long new_tail;

                p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
                                           vap, sizeof(*vap),
                                           &new_tail);
                if (p)
                        return send_tx_packet(lp, p, new_tail);
                else
                        return ldc_abort(lp);
        }
}


And the new code:

static int process_ver_nack(struct ldc_channel *lp, struct ldc_version 
*vp)
{
        struct ldc_version *vap;
        struct ldc_packet *p;
        unsigned long new_tail;

        if (vp->major == 0 && vp->minor == 0)
                return ldc_abort(lp);

        vap = find_by_major(vp->major);
        if (!vap)
                return ldc_abort(lp);

        p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
                                           vap, sizeof(*vap),
                                           &new_tail);
        if (!p)
                return ldc_abort(lp);

        return send_tx_packet(lp, p, new_tail);
}

This should be binary the same. But as I said, I do not have a sparc 
compiler to test.

Thanks,

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt - Jan. 5, 2009, 7:56 p.m.
On Mon, 5 Jan 2009, Steven Rostedt wrote:

> 
> Impact: clean up
> 
> The code in process_ver_nack is a little obfuscated. This change
> makes it a bit more readable by humans. It removes the complex
> if statement and replaces it with a cleaner flow of control.
> 
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>

Sam,

Can you test to see if this patch makes the issue go away. Obviously, Dave 
would need to be the one to pull it in, or at least ack it.

Thanks,

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sam Ravnborg - Jan. 5, 2009, 8:07 p.m.
On Mon, Jan 05, 2009 at 02:56:58PM -0500, Steven Rostedt wrote:
> 
> On Mon, 5 Jan 2009, Steven Rostedt wrote:
> 
> > 
> > Impact: clean up
> > 
> > The code in process_ver_nack is a little obfuscated. This change
> > makes it a bit more readable by humans. It removes the complex
> > if statement and replaces it with a cleaner flow of control.
> > 
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> Sam,
> 
> Can you test to see if this patch makes the issue go away. Obviously, Dave 
> would need to be the one to pull it in, or at least ack it.

As expected the patch silence the warning.
The warning only triggers when we have an assignment
after a boolean || inside an if condition.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sam Ravnborg - Jan. 5, 2009, 8:08 p.m.
On Mon, Jan 05, 2009 at 02:42:59PM -0500, Steven Rostedt wrote:
> 
> Impact: clean up
> 
> The code in process_ver_nack is a little obfuscated. This change
> makes it a bit more readable by humans. It removes the complex
> if statement and replaces it with a cleaner flow of control.
> 
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

And I can confirm the warning has dismissed with this patch.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 6, 2009, 6:23 p.m.
From: Sam Ravnborg <sam@ravnborg.org>
Date: Mon, 5 Jan 2009 21:08:48 +0100

> On Mon, Jan 05, 2009 at 02:42:59PM -0500, Steven Rostedt wrote:
> > 
> > Impact: clean up
> > 
> > The code in process_ver_nack is a little obfuscated. This change
> > makes it a bit more readable by humans. It removes the complex
> > if statement and replaces it with a cleaner flow of control.
> > 
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> And I can confirm the warning has dismissed with this patch.

Applied, thanks everyone.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/sparc/kernel/ldc.c b/arch/sparc/kernel/ldc.c
index d689823..6ce5d25 100644
--- a/arch/sparc/kernel/ldc.c
+++ b/arch/sparc/kernel/ldc.c
@@ -625,22 +625,23 @@  static int process_ver_ack(struct ldc_channel *lp, struct ldc_version *vp)
 static int process_ver_nack(struct ldc_channel *lp, struct ldc_version *vp)
 {
 	struct ldc_version *vap;
+	struct ldc_packet *p;
+	unsigned long new_tail;
 
-	if ((vp->major == 0 && vp->minor == 0) ||
-	    !(vap = find_by_major(vp->major))) {
+	if (vp->major == 0 && vp->minor == 0)
+		return ldc_abort(lp);
+
+	vap = find_by_major(vp->major);
+	if (!vap)
 		return ldc_abort(lp);
-	} else {
-		struct ldc_packet *p;
-		unsigned long new_tail;
 
-		p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
+	p = handshake_compose_ctrl(lp, LDC_INFO, LDC_VERS,
 					   vap, sizeof(*vap),
 					   &new_tail);
-		if (p)
-			return send_tx_packet(lp, p, new_tail);
-		else
-			return ldc_abort(lp);
-	}
+	if (!p)
+		return ldc_abort(lp);
+
+	return send_tx_packet(lp, p, new_tail);
 }
 
 static int process_version(struct ldc_channel *lp,