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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/01/2020 18:15, Anthony PERARD wrote:
> On Thu, Jan 23, 2020 at 06:17:06PM +0100, Roger Pau Monné wrote:
>> 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.
> For emails sent by your MUA, I have:
> Content-Type: text/plain; charset="iso-8859-1"
> There's nothing wrong with that, my MUA uses the same charset. If, in the
> patch that I try to apply, I replace the content-type line of the patch
> by the one from the email header, git applied the patch just fine and
> don't complain.
>
> So, the email encoding is fine.
>
> It is just the copy of an email into another email's body that was an
> issue.
>
>>> 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.
> The email I received was just fine, encoded properly (I think). It is
> just trying to apply the patch embedded into the body of the email that
> was annoying.
>
>>>> 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?
> That's not going to work.
>
>> Not sure that's any better than just using an empty one.
> And an "empty one" doesn't work either, the dcs created here contains
> more that just the `build_state' and `guest_config', it also contains
> for all the _fd field set to something.
>
> The _fd thing is important because Andrew check the value of
> `restore_fd' to figure out if a domain is been restored or not.
>
> I don't have better suggestion for now, I'll try to think about it.

So I did discuss my patch being horrible in the commit message
commentary, but got an ack without any further discussion.  The structs
vs "where useful information is" map is chronically twisted.

An alternative would be to pass down an explicit "clean build" vs
"restore from serialised state" flag, which is the information actually
needed.

~Andrew

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