Re: [Xen-devel] [xen-unstable bisection] complete test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 23, 2020 at 03:34:25PM +0000, Anthony PERARD wrote:
> On Tue, Jan 21, 2020 at 10:21:09AM +0000, Roger Pau Monné wrote:
> > The issue is that this change is passing the guest domain_create_state
> > to libxl__domain_build in libxl__spawn_stub_dm, and hence the
> > stubdomain doesn't get created. I have the following patch that fixes
> > it, but it's kind of dirty.
> > 
> > ---8<---
> > From 688fde95992d07bb1123d324a68006dd08bc6512 Mon Sep 17 00:00:00 2001
> > From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > Date: Tue, 21 Jan 2020 10:14:09 +0000
> > Subject: [PATCH] libxl: fix stubdomain creation after aacc143006429de
> > MIME-Version: 1.0
> > Content-Type: text/plain; charset=UTF-8
> 
> :-(, this is a lie. The email I've received has a different charset.

Really? The email headers also contain the same tag, and hence all my
emails would have a wrong encoding then.

> git
> complained about it. Maybe next time the patch could be attached, or it
> could be a proper patch with some note (after ---) (git send-email can
> do --in-reply-to), or it could be two separated emails with the first
> one replying to the report and the second the patch (all in the same
> thread).

I can certainly send the patch separately as a reply as you say above,
but I would still need to fix my email client to set the proper
encoding then.

> 
> > Content-Transfer-Encoding: 8bit
> > 
> > aacc143006429de broke stubdomain creation by passing the guest
> > domain_create_state to libxl__domain_build in libxl__spawn_stub_dm,
> > when it should instead be crafting a new domain_create_state for the
> > stubdomain.
> > 
> > Fixes: aacc143006429de ('tools/libxl: Plumb domain_create_state down into 
> > libxl__build_pre()')
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  tools/libxl/libxl_dm.c       | 22 +++++++++++++---------
> >  tools/libxl/libxl_internal.h |  3 +--
> >  2 files changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 3f08ccad1b..b1ddde77e8 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -2110,17 +2110,21 @@ void libxl__spawn_stub_dm(libxl__egc *egc, 
> > libxl__domain_create_state *dcs)
> >      xs_transaction_t t;
> >  
> >      /* convenience aliases */
> > -    libxl_domain_config *const dm_config = &sdss->dm_config;
> >      libxl_domain_config *const guest_config = sdss->dm.guest_config;
> >      const int guest_domid = sdss->dm.guest_domid;
> >      libxl__domain_build_state *const d_state = sdss->dm.build_state;
> > -    libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
> > +    libxl__domain_build_state *stubdom_state;
> > +    libxl_domain_config *dm_config;
> >  
> >      /* Initialise private part of sdss */
> > -    libxl__domain_build_state_init(stubdom_state);
> >      dmss_init(&sdss->dm);
> >      dmss_init(&sdss->pvqemu);
> >      libxl__xswait_init(&sdss->xswait);
> > +    GCNEW(sdss->dcs);
> > +    stubdom_state = &sdss->dcs->build_state;
> > +    libxl__domain_build_state_init(stubdom_state);
> > +    GCNEW(sdss->dcs->guest_config);
> > +    dm_config = sdss->dcs->guest_config;
> 
> I don't think that's enough, we need to initialize the dcs properly.
> Otherwise, libxl__domain_build() might start using thing that aren't set
> properly. Maybe we would need a new struct which could be pass to
> libxl__domain_build*, or that might be more complicated than needed.

Er likely yes, but creating a complete domain_create_state for the
stubdom will be very cumbersome I think. Maybe we can copy the one
from the guest over the stubdom one in order to initialize it?

Not sure that's any better than just using an empty one.

> >  
> >      if (guest_config->b_info.device_model_version !=
> >          LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index d919f91882..abf88dfd76 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -4102,8 +4102,7 @@ typedef struct {
> >      /* filled in by user, must remain valid: */
> >      libxl__dm_spawn_cb *callback; /* called as callback(,&sdss->dm,) */
> >      /* private to libxl__spawn_stub_dm: */
> > -    libxl_domain_config dm_config;
> > -    libxl__domain_build_state dm_state;
> > +    libxl__domain_create_state *dcs;
> 
> This should be named dm_dcs, I think, to follow the same pattern as
> before.

Sure.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel