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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Mar-24 17:53 UTC
Re: [Xen-devel] [PATCH 1/3] xen: Control-flow cleanup
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; > } > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel