Re: [PATCH v2 2/2] hw/arm/smmu-common: Simplify smmu_find_smmu_pcibus() logic

[Auger Eric]

Hi Philippe,
On 2/27/20 8:51 AM, Philippe Mathieu-Daudé wrote:
> The smmu_find_smmu_pcibus() function was introduced (in commit
> cac994ef43b) in a code format that could return an incorrect
> pointer, which was then fixed by the previous commit.
> We could have avoid this by writing the if() statement differently.
nit: avoided
> Do it now, in case this function is re-used. The code is easier to
> review (harder to miss bugs).
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> This patch is easier to review with 'git-diff -w' (--ignore-all-space)
> ---
>  hw/arm/smmu-common.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 67d7b2d0fd..e13a5f4a7c 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -290,20 +290,21 @@ inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, 
> IOMMUAccessFlags perm,
>  SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num)
>  {
>      SMMUPciBus *smmu_pci_bus = s->smmu_pcibus_by_bus_num[bus_num];
> +    GHashTableIter iter;
>  
> -    if (!smmu_pci_bus) {
> -        GHashTableIter iter;
> -
> -        g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
> -        while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
> -            if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
> -                s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
> -                return smmu_pci_bus;
> -            }
> -        }
> -        smmu_pci_bus = NULL;
> +    if (smmu_pci_bus) {
> +        return smmu_pci_bus;
>      }
> -    return smmu_pci_bus;
> +
> +    g_hash_table_iter_init(&iter, s->smmu_pcibus_by_busptr);
> +    while (g_hash_table_iter_next(&iter, NULL, (void **)&smmu_pci_bus)) {
> +        if (pci_bus_num(smmu_pci_bus->bus) == bus_num) {
> +            s->smmu_pcibus_by_bus_num[bus_num] = smmu_pci_bus;
> +            return smmu_pci_bus;
> +        }
> +    }
> +
> +    return NULL;
>  }
>  
>  static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
> 
Acked-by: Eric Auger <address@hidden>

Thanks!

Eric