Patchwork Re: [PATCH 20/20] pci: remove goto in pci_bridge_filter().

login
register
mail settings
Submitter Michael S. Tsirkin
Date Nov. 12, 2009, 12:08 p.m.
Message ID <20091112120805.GC11106@redhat.com>
Download mbox | patch
Permalink /patch/38244/
State New
Headers show

Comments

Michael S. Tsirkin - Nov. 12, 2009, 12:08 p.m.
On Thu, Nov 12, 2009 at 02:58:48PM +0900, Isaku Yamahata wrote:
> This patch removes ugly goto in pci_bridge_filter() by
> introducing subfunction, pci_bridge_filter_nomap().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

goto on error is actually cleaner IMO.
just *not into scope*.

> ---
>  hw/pci.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index add919b..90bdf5e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -691,6 +691,12 @@ static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
>      return limit;
>  }
>  
> +static void pci_bridge_filter_nomap(pcibus_t *addr, pcibus_t *size)
> +{
> +    *addr = PCI_BAR_UNMAPPED;
> +    *size = 0;
> +}
> +
>  static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>                                uint8_t type)
>  {
> @@ -703,11 +709,13 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>  
>          if (type & PCI_BASE_ADDRESS_SPACE_IO) {
>              if (!(cmd & PCI_COMMAND_IO)) {
> -                goto no_map;
> +                pci_bridge_filter_nomap(addr, size);
> +                return;
>              }
>          } else {
>              if (!(cmd & PCI_COMMAND_MEMORY)) {
> -                goto no_map;
> +                pci_bridge_filter_nomap(addr, size);
> +                return;
>              }
>          }
>  
> @@ -716,9 +724,7 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>      }
>  
>      if (base > limit) {
> -    no_map:
> -        *addr = PCI_BAR_UNMAPPED;
> -        *size = 0;
> +        pci_bridge_filter_nomap(addr, size);
>      } else {
>          *addr = base;
>          *size = limit - base + 1;

Here's what I came up with:

--->
From: Michael S. Tsirkin <mst@redhat.com>
Subject: pci: convert goto into scope in bridge_filter
    
goto into scope is evil. rearrange pci_bridge_filter
so that we always go to end of function on error.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---
Isaku Yamahata - Nov. 12, 2009, 1:13 p.m.
On Thu, Nov 12, 2009 at 02:08:05PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2009 at 02:58:48PM +0900, Isaku Yamahata wrote:
> > This patch removes ugly goto in pci_bridge_filter() by
> > introducing subfunction, pci_bridge_filter_nomap().
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> goto on error is actually cleaner IMO.
> just *not into scope*.
> 
> > ---
> >  hw/pci.c |   16 +++++++++++-----
> >  1 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index add919b..90bdf5e 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -691,6 +691,12 @@ static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
> >      return limit;
> >  }
> >  
> > +static void pci_bridge_filter_nomap(pcibus_t *addr, pcibus_t *size)
> > +{
> > +    *addr = PCI_BAR_UNMAPPED;
> > +    *size = 0;
> > +}
> > +
> >  static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
> >                                uint8_t type)
> >  {
> > @@ -703,11 +709,13 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
> >  
> >          if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> >              if (!(cmd & PCI_COMMAND_IO)) {
> > -                goto no_map;
> > +                pci_bridge_filter_nomap(addr, size);
> > +                return;
> >              }
> >          } else {
> >              if (!(cmd & PCI_COMMAND_MEMORY)) {
> > -                goto no_map;
> > +                pci_bridge_filter_nomap(addr, size);
> > +                return;
> >              }
> >          }
> >  
> > @@ -716,9 +724,7 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
> >      }
> >  
> >      if (base > limit) {
> > -    no_map:
> > -        *addr = PCI_BAR_UNMAPPED;
> > -        *size = 0;
> > +        pci_bridge_filter_nomap(addr, size);
> >      } else {
> >          *addr = base;
> >          *size = limit - base + 1;
> 
> Here's what I came up with:
> 
> --->
> From: Michael S. Tsirkin <mst@redhat.com>
> Subject: pci: convert goto into scope in bridge_filter
>     
> goto into scope is evil. rearrange pci_bridge_filter
> so that we always go to end of function on error.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Acked-by: Isaku Yamahata <yamahata@valinux.co.jp>


> 
> ---
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 14de2d1..6e5d57b 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -716,13 +716,14 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>      }
>  
>      if (base > limit) {
> -    no_map:
> -        *addr = PCI_BAR_UNMAPPED;
> -        *size = 0;
> -    } else {
> -        *addr = base;
> -        *size = limit - base + 1;
> +        goto no_map;
>      }
> +    *addr = base;
> +    *size = limit - base + 1;
> +    return;
> +no_map:
> +    *addr = PCI_BAR_UNMAPPED;
> +    *size = 0;
>  }
>  
>  static pcibus_t pci_bar_address(PCIDevice *d,
>

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 14de2d1..6e5d57b 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -716,13 +716,14 @@  static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
     }
 
     if (base > limit) {
-    no_map:
-        *addr = PCI_BAR_UNMAPPED;
-        *size = 0;
-    } else {
-        *addr = base;
-        *size = limit - base + 1;
+        goto no_map;
     }
+    *addr = base;
+    *size = limit - base + 1;
+    return;
+no_map:
+    *addr = PCI_BAR_UNMAPPED;
+    *size = 0;
 }
 
 static pcibus_t pci_bar_address(PCIDevice *d,