Re: [PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain [and 1 more messages]

[Jason Andryuk <jandryuk@gmail.com>]

On Mon, May 18, 2020 at 11:18 AM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> >
> Marek Marczykowski-Górecki writes ("Re: [PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> > On Mon, May 18, 2020 at 03:15:17PM +0100, Ian Jackson wrote:
> > > Err, by "the qemu savefile pathname" I meant the pathname in dom0.
> > > I assume your wrapper script opens that and feeds it to the console.
> > > Is that right ?
> >
> > Not really a wrapper script. On dom0 side it is console backend (qemu)
> > instructed to connect the console to a file, instead of pty. I have
> > implemented similar feature in my xenconsoled patch series sent a while
> > ago (sent along with v2 of this series), but that series needs some more
> > love.
> >
> > On the stubdomain side, it is a script that launches qemu - opens a
> > /dev/hvc2, then pass the FD to qemu via -incoming option (which is
> > really constructed by libxl).
>
> Hi.  Thanks for trying to help me understand.  I was still confused
> though.  I tried to explain another way and that helped me see what's
> going on.
>
> I think I understand now.
>
> For reference, my confusion was this:
>
> Jason Andryuk writes ("[PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> > index bdc23554eb..45d0dd56f5 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1744,10 +1744,17 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> >      }
> >
> >      if (state->saved_state) {
> > -        /* This file descriptor is meant to be used by QEMU */
> > -        *dm_state_fd = open(state->saved_state, O_RDONLY);
> > -        flexarray_append(dm_args, "-incoming");
> > -        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> > +        if (is_stubdom) {
> > +            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
> > +             */
> > +            flexarray_append(dm_args, "-incoming");
> > +            flexarray_append(dm_args, "fd:3");
> > +        } else {
> > +            /* This file descriptor is meant to be used by QEMU */
> > +            *dm_state_fd = open(state->saved_state, O_RDONLY);
> > +            flexarray_append(dm_args, "-incoming");
> > +            flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> > +        }
>
> In this hunk, the call
>            *dm_state_fd = open(state->saved_state, O_RDONLY);
> becomes conditional.  It is no longer executed in the stubdomain
> case.
>
> So then, who opens state->saved_state ?  And how do they get told
> where it is ?  If it's somewhere else in libxl, why doesn't it show up
> in this patch ?
>
> Posing the question liked that allowed me to see that the answer is
>
>            console[i].output = GCSPRINTF("file:%s",
>                            libxl__device_model_savefile(gc, guest_domid));
>
> in spawn_stub_launch_dm.  And it doesn't appear in the patch because
> it's already used for minios stub trad qemu and the same code is
> now going to be executed for linux stub moderm qemu.

Do you want the commit message to add a blurb about this?  So the
message becomes:
"""
Rely on a wrapper script in stubdomain to attach relevant consoles to
qemu.  The save console (1) must be attached to fdset/1.  When
performing a restore, $STUBDOM_RESTORE_INCOMING_ARG must be replaced on
the qemu command line by "fd:$FD", where $FD is an open file descriptor
number to the restore console (2).

Existing libxl code (for dom0) already connects the stubdom's save &
restore console outputs to the save & restore files.
"""

-Jason