[PATCH 1/3] xen: Control-flow cleanup

[PATCH 1/3] xen: Control-flow cleanup

[Bastian Blank]

It is nice to have only one return, but it does not make it easier to
read or change.

Signed-off-by: Bastian Blank <waldi@debian.org>
---
 arch/x86/xen/pci.c |   52 +++++++++++++++++++++++++++-------------------------
 1 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/arch/x86/xen/pci.c b/arch/x86/xen/pci.c
index 61e1ade..a83c74c 100644
--- a/arch/x86/xen/pci.c
+++ b/arch/x86/xen/pci.c
@@ -42,32 +42,34 @@ int xen_register_gsi(u32 gsi, int triggering, int polarity)
 
 	printk(KERN_DEBUG "xen: --> irq=%d\n", irq);
 
-	if (irq >= 0) {
-		setup_gsi.gsi = gsi;
-		setup_gsi.triggering = (triggering == ACPI_EDGE_SENSITIVE ?
-				0 : 1);
-		setup_gsi.polarity = (polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
-
-		rc = HYPERVISOR_physdev_op(PHYSDEVOP_setup_gsi, &setup_gsi);
-		if (rc == -EEXIST)
-			printk(KERN_INFO "Already setup the GSI :%d\n", gsi);
-		else if (rc) {
-			printk(KERN_ERR "Failed to setup GSI :%d, err_code:%d\n",
-					gsi, rc);
-			BUG();
-		}
+	if (irq < 0)
+		return irq;
+
+	setup_gsi.gsi = gsi;
+	setup_gsi.triggering = (triggering == ACPI_EDGE_SENSITIVE ?
+			0 : 1);
+	setup_gsi.polarity = (polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
+
+	rc = HYPERVISOR_physdev_op(PHYSDEVOP_setup_gsi, &setup_gsi);
+	if (rc == -EEXIST)
+		printk(KERN_INFO "Already setup the GSI :%d\n", gsi);
+	else if (rc) {
+		printk(KERN_ERR "Failed to setup GSI :%d, err_code:%d\n",
+				gsi, rc);
+		BUG();
+	}
 
-		map_irq.domid = DOMID_SELF;
-		map_irq.type = MAP_PIRQ_TYPE_GSI;
-		map_irq.index = gsi;
-		map_irq.pirq = irq;
+	map_irq.domid = DOMID_SELF;
+	map_irq.type = MAP_PIRQ_TYPE_GSI;
+	map_irq.index = gsi;
+	map_irq.pirq = irq;
 
-		rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
-		if (rc) {
-			printk(KERN_WARNING "xen map irq failed %d\n", rc);
-			irq = -1;
-		}
+	rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq, &map_irq);
+	if (rc) {
+		printk(KERN_WARNING "xen map irq failed %d\n", rc);
+		return -1;
 	}
+
 	return irq;
 }
 
-- 
1.7.0


-- 
Actual war is a very messy business.  Very, very messy business.
		-- Kirk, "A Taste of Armageddon", stardate 3193.0

Re: [PATCH 1/3] xen: Control-flow cleanup

[Jeremy Fitzhardinge]

On 03/24/2010 04:51 AM, Bastian Blank wrote:
> It is nice to have only one return, but it does not make it easier to
> read or change.
>    

Thanks, I agree, but I tweaked it to use "goto out" to preserve the 
single return, and fixed some lines which can now be unwrapped with the 
shallower indent.

     J

> Signed-off-by: Bastian Blank<waldi@debian.org>
> ---
>   arch/x86/xen/pci.c |   52 +++++++++++++++++++++++++++-------------------------
>   1 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/xen/pci.c b/arch/x86/xen/pci.c
> index 61e1ade..a83c74c 100644
> --- a/arch/x86/xen/pci.c
> +++ b/arch/x86/xen/pci.c
> @@ -42,32 +42,34 @@ int xen_register_gsi(u32 gsi, int triggering, int polarity)
>
>   	printk(KERN_DEBUG "xen: -->  irq=%d\n", irq);
>
> -	if (irq>= 0) {
> -		setup_gsi.gsi = gsi;
> -		setup_gsi.triggering = (triggering == ACPI_EDGE_SENSITIVE ?
> -				0 : 1);
> -		setup_gsi.polarity = (polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
> -
> -		rc = HYPERVISOR_physdev_op(PHYSDEVOP_setup_gsi,&setup_gsi);
> -		if (rc == -EEXIST)
> -			printk(KERN_INFO "Already setup the GSI :%d\n", gsi);
> -		else if (rc) {
> -			printk(KERN_ERR "Failed to setup GSI :%d, err_code:%d\n",
> -					gsi, rc);
> -			BUG();
> -		}
> +	if (irq<  0)
> +		return irq;
> +
> +	setup_gsi.gsi = gsi;
> +	setup_gsi.triggering = (triggering == ACPI_EDGE_SENSITIVE ?
> +			0 : 1);
> +	setup_gsi.polarity = (polarity == ACPI_ACTIVE_HIGH ? 0 : 1);
> +
> +	rc = HYPERVISOR_physdev_op(PHYSDEVOP_setup_gsi,&setup_gsi);
> +	if (rc == -EEXIST)
> +		printk(KERN_INFO "Already setup the GSI :%d\n", gsi);
> +	else if (rc) {
> +		printk(KERN_ERR "Failed to setup GSI :%d, err_code:%d\n",
> +				gsi, rc);
> +		BUG();
> +	}
>
> -		map_irq.domid = DOMID_SELF;
> -		map_irq.type = MAP_PIRQ_TYPE_GSI;
> -		map_irq.index = gsi;
> -		map_irq.pirq = irq;
> +	map_irq.domid = DOMID_SELF;
> +	map_irq.type = MAP_PIRQ_TYPE_GSI;
> +	map_irq.index = gsi;
> +	map_irq.pirq = irq;
>
> -		rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq,&map_irq);
> -		if (rc) {
> -			printk(KERN_WARNING "xen map irq failed %d\n", rc);
> -			irq = -1;
> -		}
> +	rc = HYPERVISOR_physdev_op(PHYSDEVOP_map_pirq,&map_irq);
> +	if (rc) {
> +		printk(KERN_WARNING "xen map irq failed %d\n", rc);
> +		return -1;
>   	}
> +
>   	return irq;
>   }
>
>    

Re: [PATCH 1/3] xen: Control-flow cleanup

[Bastian Blank]

On Wed, Mar 24, 2010 at 10:53:13AM -0700, Jeremy Fitzhardinge wrote:
> Thanks, I agree, but I tweaked it to use "goto out" to preserve the  
> single return, and fixed some lines which can now be unwrapped with the  
> shallower indent.

What is the reason for this single return except that it makes the code
less readable?

Bastian

-- 
Totally illogical, there was no chance.
		-- Spock, "The Galileo Seven", stardate 2822.3