Re: [PATCH v2 2/5] pci: Always pass own DeviceState to pci_map_irq_fn's

[Peter Maydell]

On Sat, 12 Feb 2022 at 13:27, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> > Passing own DeviceState rather than just the IRQs allows for resolving
> > global variables.
>
> Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?
>
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> > hw/isa/piix4.c          | 6 +++---
> > hw/pci-host/sh_pci.c    | 6 +++---
> > hw/pci-host/versatile.c | 6 +++---
> > hw/ppc/ppc440_pcix.c    | 6 +++---
> > hw/ppc/ppc4xx_pci.c     | 6 +++---
> > 5 files changed, 15 insertions(+), 15 deletions(-)
>
> You don't seem to change any global function definition that reqires this
> change in all these devices so why can't these decide on their own what
> their preferred opaque data is for their set irq function and only change
> piix4? Am I missing something?

Yeah, we don't necessarily need to change all these devices,
but I think this is an area where over time we're shifting from
an older school of API design that was "we have some basically
standalone functions that take an arbitrary opaque pointer"
towards a more object-oriented design, where the opaque pointer
should probably be the pointer-to-this-object unless there's a good
reason why it needs to be something else[*], because having a function
that's part of a device being passed something other than the
device-instance pointer is a bit unexpected.

In some cases there is a good reason for opaque pointers for
function callbacks to be something else, because passing in the
device pointer would make the code noticeably more complicated.
But in the specific cases here, the only change is that the
callback functions use "s->somearray[i]" instead of
"an_argument[i]", which doesn't seem to me like a significant
complexity change.

thanks
-- PMM