[v2,3/6] qtest: implement named interception of out-GPIO

Adds qtest_irq_intercept_out_named method, which utilizes a new optional
name parameter to the irq_intercept_out qtest command.

Signed-off-by: Chris Laplante <chris@laplante.io>
---
 softmmu/qtest.c        | 24 ++++++++++++++++--------
 tests/qtest/libqtest.c |  6 ++++++
 tests/qtest/libqtest.h | 11 +++++++++++
 3 files changed, 33 insertions(+), 8 deletions(-)

On Wed, 26 Jul 2023 at 04:32, Chris Laplante <chris@laplante.io> wrote:
>
> Adds qtest_irq_intercept_out_named method, which utilizes a new optional
> name parameter to the irq_intercept_out qtest command.
>
> Signed-off-by: Chris Laplante <chris@laplante.io>
> ---
>  softmmu/qtest.c        | 24 ++++++++++++++++--------
>  tests/qtest/libqtest.c |  6 ++++++
>  tests/qtest/libqtest.h | 11 +++++++++++
>  3 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/softmmu/qtest.c b/softmmu/qtest.c
> index 1c92e5a6a3..7fd8546ed2 100644
> --- a/softmmu/qtest.c
> +++ b/softmmu/qtest.c
> @@ -397,8 +397,12 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>          || strcmp(words[0], "irq_intercept_in") == 0) {
>          DeviceState *dev;
>          NamedGPIOList *ngl;
> +        bool is_named;
> +        bool is_outbound;
>
>          g_assert(words[1]);
> +        is_named = words[2] != NULL;
> +        is_outbound = words[0][14] == 'o';
>          dev = DEVICE(object_resolve_path(words[1], NULL));
>          if (!dev) {
>              qtest_send_prefix(chr);
> @@ -417,14 +421,18 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>          }
>
>          QLIST_FOREACH(ngl, &dev->gpios, node) {
> -            /* We don't support intercept of named GPIOs yet */
> -            if (ngl->name) {
> -                continue;
> -            }
> -            if (words[0][14] == 'o') {
> -                int i;
> -                for (i = 0; i < ngl->num_out; ++i) {
> -                    qtest_install_gpio_out_intercept(dev, ngl->name, i);
> +            /* We don't support inbound interception of named GPIOs yet */
> +            if (is_outbound) {
> +                if (is_named) {
> +                    if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
> +                        qtest_install_gpio_out_intercept(dev, ngl->name, 0);
> +                        break;
> +                    }

Named gpio-outs can have more than one line, the same as
unnamed ones (you create them with
qdev_init_gpio_out_named(dev, pins, name, n) -- there's an
argument for how many to create), so I think this is_named
branch also needs to install an intercept for each one, not
just for 0.

> +                } else if (!ngl->name) {
> +                    int i;
> +                    for (i = 0; i < ngl->num_out; ++i) {
> +                        qtest_install_gpio_out_intercept(dev, ngl->name, i);
> +                    }

...at which point the code looks pretty similar in both branches
of the if(), so I think you end up with something like

         if (is_outbound) {
             /* NULL is valid and matchable, for "unnamed GPIO" */
             if (g_strcmp0(ngl->name, words[2]) == 0) {
                 int i;
;                for (i = 0; i < ngl->num_out; ++i) {
                     qtest_install_gpio_out_intercept(dev, ngl->name, i);
                 }
             }
         } ...

(g_strcmp0() can handle the NULL case without having
to special case it -- this is how qdev_get_named_gpio_list()
finds entries in the ngl list.)

Apologies for not noticing that on the first round of review.

thanks
-- PMM

> (g_strcmp0() can handle the NULL case without having
> to special case it -- this is how qdev_get_named_gpio_list()
> finds entries in the ngl list.)
> 
> Apologies for not noticing that on the first round of review.

No worries - it makes the code much simpler anyway. Should we bother factoring out qtest_install_gpio_out_intercept still? It is only used in one place now, as before.

Thanks,
Chris

On Thu, 27 Jul 2023 at 22:32, Chris Laplante <chris@laplante.io> wrote:
>
>
> > (g_strcmp0() can handle the NULL case without having
> > to special case it -- this is how qdev_get_named_gpio_list()
> > finds entries in the ngl list.)
> >
> > Apologies for not noticing that on the first round of review.
>
> No worries - it makes the code much simpler anyway. Should we bother factoring out qtest_install_gpio_out_intercept still? It is only used in one place now, as before.

I'd keep the function, I think, since you've already written
it. It's kind of usefully documenting of the intent.

-- PMM