I've observed this failed assertion:
libxl_event.c:2057: libxl__ao_inprogress_gc: Assertion `ao' failed.

AFAICT, this is happening in qmp_proxy_spawn_outcome where
sdss->qmp_proxy_spawn.ao is NULL.

The out label of spawn_stub_launch_dm calls qmp_proxy_spawn_outcome, but
it is only in the success path that sdss->qmp_proxy_spawn.ao gets set to
the current ao.  Move the setting earlier to have an ao in all paths
through the function.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Another option would be to make spawn_stub_launch_dm call
spawn_stubdom_pvqemu_cb on error.  This avoids needing to set
sdss->qmp_proxy_spawn.ao, but it makes more paths through the code.
---
 tools/libs/light/libxl_dm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 9a8ddbe188..59a8dcf3a9 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2468,6 +2468,9 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
     uint32_t dm_domid = sdss->pvqemu.guest_domid;
     int need_qemu;
 
+    /* Set for out label through qmp_proxy_spawn_outcome(). */
+    sdss->qmp_proxy_spawn.ao = ao;
+
     if (ret) {
         LOGD(ERROR, guest_domid, "error connecting disk devices");
         goto out;
@@ -2567,7 +2570,6 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
             goto out;
     }
 
-    sdss->qmp_proxy_spawn.ao = ao;
     if (libxl__stubdomain_is_linux(&guest_config->b_info)) {
         spawn_qmp_proxy(egc, sdss);
     } else {
-- 
2.35.1

On Wed, Mar 30, 2022 at 02:16:58PM -0400, Jason Andryuk wrote:
> diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> index 9a8ddbe188..59a8dcf3a9 100644
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -2468,6 +2468,9 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>      uint32_t dm_domid = sdss->pvqemu.guest_domid;
>      int need_qemu;
>  
> +    /* Set for out label through qmp_proxy_spawn_outcome(). */
> +    sdss->qmp_proxy_spawn.ao = ao;

I don't think that's correct. I feels like `sdss->qmp_proxy_spawn`
doesn't need to be initialised before calling spawn_qmp_proxy().

Also `qmp_proxy_spawn.ao` only need to be set before calling
libxl__spawn_spawn(), so at the same time as the rest of the
initialisation of `qmp_proxy_spawn` in spawn_qmp_proxy().


Next, about the uninitialized `ao` field:
- qmp_proxy_spawn_outcome() shouldn't use `qmp_proxy_spawn.ao`, but
  instead it should use the one available in `sdss`, that is
  `sdss->dm.spawn.ao` (The one that libxl__spawn_stub_dm() or
  spawn_stub_launch_dm() is using).
- And spawn_qmp_proxy() should also use `sdss->dm.spawn.ao` for
  STATE_AO_GC() as I don't think spawn_qmp_proxy() can expect
  `qmp_proxy_spawn` to be initialised as that's the function that
  initialise it.


Thanks,

-- 
Anthony PERARD

On Thu, Mar 31, 2022 at 9:24 AM Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> On Wed, Mar 30, 2022 at 02:16:58PM -0400, Jason Andryuk wrote:
> > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> > index 9a8ddbe188..59a8dcf3a9 100644
> > --- a/tools/libs/light/libxl_dm.c
> > +++ b/tools/libs/light/libxl_dm.c
> > @@ -2468,6 +2468,9 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >      uint32_t dm_domid = sdss->pvqemu.guest_domid;
> >      int need_qemu;
> >
> > +    /* Set for out label through qmp_proxy_spawn_outcome(). */
> > +    sdss->qmp_proxy_spawn.ao = ao;
>
> I don't think that's correct. I feels like `sdss->qmp_proxy_spawn`
> doesn't need to be initialised before calling spawn_qmp_proxy().
>
> Also `qmp_proxy_spawn.ao` only need to be set before calling
> libxl__spawn_spawn(), so at the same time as the rest of the
> initialisation of `qmp_proxy_spawn` in spawn_qmp_proxy().
>
>
> Next, about the uninitialized `ao` field:
> - qmp_proxy_spawn_outcome() shouldn't use `qmp_proxy_spawn.ao`, but
>   instead it should use the one available in `sdss`, that is
>   `sdss->dm.spawn.ao` (The one that libxl__spawn_stub_dm() or
>   spawn_stub_launch_dm() is using).
> - And spawn_qmp_proxy() should also use `sdss->dm.spawn.ao` for
>   STATE_AO_GC() as I don't think spawn_qmp_proxy() can expect
>   `qmp_proxy_spawn` to be initialised as that's the function that
>   initialise it.

That all sounds good.  Thanks, Anthony.

-Jason