| Message ID | 20220330181658.30209-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl: Always set ao for qmp_proxy_spawn_outcome | expand |
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,
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
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 {
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(-)