Re: [PATCH 3/3] tools/xl: move some logic needed only once earlier in

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Fri, Oct 16, 2026 at 01:07:21PM +0800, chunjie.zhu@xxxxxxxxxx wrote:
> On Mon, Oct 12, 2026 at 06:03:49 PM, Elliott Mitchell <ehem+xen@xxxxxxx> 
> wrote:
> 
> > The value of dom_info->console_autoconnect isn't going to change if a
> > domain is rebooted.  Nor does this logic need the lock to be held.  As
> > such moving this earlier makes sense.
> >
> > The policy domain is set once and then never changed.  Similar to above
> > simply move before the reboot start point.
> >
> > Signed-off-by: Elliott Mitchell <ehem+xen@xxxxxxx>
> > ---
> >  tools/xl/xl_vmcontrol.c | 32 +++++++++++++++-----------------
> >  1 file changed, 15 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> > index 76c2a07ee8..ae7985122f 100644
> > --- a/tools/xl/xl_vmcontrol.c
> > +++ b/tools/xl/xl_vmcontrol.c
> > @@ -717,7 +717,8 @@ int create_domain(struct domain_create *dom_info)
> >      int restore_fd = -1;
> >      int restore_fd_to_close = -1;
> >      int send_back_fd = -1;
> > -    const libxl_asyncprogress_how *autoconnect_console_how;
> > +    const libxl_asyncprogress_how *autoconnect_console_how = NULL;
> > +    libxl_asyncprogress_how autoconnect_console_how_buf;
> >      int notify_pipe[2] = { -1, -1 };
> >      struct save_file_header hdr;
> >      uint32_t domid_soft_reset = INVALID_DOMID;
> > @@ -921,6 +922,19 @@ int create_domain(struct domain_create *dom_info)
> >      if (dryrun_only)
> >          goto out;
> >
> > +    if (dom_info->console_autoconnect) {
> > +        if (libxl_pipe(ctx, notify_pipe)) {
> > +            ret = ERROR_FAIL;
> > +            goto error_out;
> > +        }
> 
> It looks like a functional regression for the reboot path. Right before
> "goto start;" on the guest-reboot path there is:
> 
>     /*
>      * Do not attempt to reconnect if we come round again due to a
>      * guest reboot -- the stdin/out will be disconnected by then.
>      */
>     dom_info->console_autoconnect = 0;
> 
> On reboot, dom_info->console_autoconnect is cleared before "goto start",
> but autoconnect_console_how now stays non-NULL, since it is no longer
> recomputed after "start:". The second create call will invoke
> autoconnect_console with the already-closed notify_pipe[1], and the
> read(notify_pipe[0]) will fail with EBADF.

Yeah, I spotted this before you sent the message, but didn't get things
done before you'd sent the message.

It gets worse.  I'm pretty sure the patch is mostly correct, but this
needed to be propogated further.  The one and only consequence of the
above hunk is to cause the `autoconnect_console_how = 0;` line to
trigger.

In turn the one and only consequence of that setting is
`libxl_domain_soft_reset()` gets passed 0/NULL as its final argument.
The `libxl_domain_soft_reset()` call is only done on domain reboots.
Worse, `libxl_domain_soft_reset()` is *always* done on domain reboots.

I've got the patch in my tree with the hunk you noticed removed and the
call to `libxl_domain_soft_reset()` hard-coding the final argument as
NULL.  The only question is whether the comment should be preserved just
before the `libxl_domain_soft_reset()` call.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445