Re: [PATCH] hw/arm/smmu-common: a fix to smmu_find_smmu_pcibus

[Peter Maydell]

On Wed, 26 Feb 2020 at 17:37, Philippe Mathieu-Daudé <address@hidden> wrote:
>
> Hi Eric,

> Patch is easy to review but code not. By inverting the if() statement I
> find the code easier to review.



> 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) {
>          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;
> }
>
> What do you think?

I think I agree with Philippe here -- the current code is already
a bit confusing in that it looks at first glance as if the "find
the bus in the hash table" code works by falling through into
the "we already had the cached value" code on success, but in
fact the success case is dealt with by the return in the middle
of the while loop and it's only the not-found case that falls
through. Adding this patch on top fixes the bug but leaves in
place the odd code structure that caused it. Rearranging it as
Philippe does above makes it much clearer what's happening,
I think.

Would one of you like to submit a patch that does it that way
round, please?

thanks
-- PMM