Re: [PATCH] libxl: only query VNC when enabled

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Oct 8, 2020 at 9:31 AM Jason Andryuk <jandryuk@xxxxxxxxx> wrote:
>
> On Wed, Oct 7, 2020 at 6:50 AM Wei Liu <wl@xxxxxxx> wrote:
> >
> > On Thu, Oct 01, 2020 at 07:53:37PM -0400, Jason Andryuk wrote:
> > > QEMU without VNC support (configure --disable-vnc) will return an error
> > > when VNC is queried over QMP since it does not recognize the QMP
> > > command.  This will cause libxl to fail starting the domain even if VNC
> > > is not enabled.  Therefore only query QEMU for VNC support when using
> > > VNC, so a VNC-less QEMU will function in this configuration.
> > >
> > > 'goto out' jumps to the call to device_model_postconfig_done(), the
> > > final callback after the chain of vnc queries.  This bypasses all the
> > > QMP VNC queries.
> > >
> > > Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> > > ---
> > >  tools/libs/light/libxl_dm.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> > > index a944181781..d1ff35dda3 100644
> > > --- a/tools/libs/light/libxl_dm.c
> > > +++ b/tools/libs/light/libxl_dm.c
> > > @@ -3140,6 +3140,7 @@ static void 
> > > device_model_postconfig_chardev(libxl__egc *egc,
> > >  {
> > >      EGC_GC;
> > >      libxl__dm_spawn_state *dmss = CONTAINER_OF(qmp, *dmss, qmp);
> > > +    const libxl_vnc_info *vnc = libxl__dm_vnc(dmss->guest_config);
> > >      const libxl__json_object *item = NULL;
> > >      const libxl__json_object *o = NULL;
> > >      int i = 0;
> > > @@ -3197,6 +3198,9 @@ static void 
> > > device_model_postconfig_chardev(libxl__egc *egc,
> > >          if (rc) goto out;
> > >      }
> > >
> > > +    if (!vnc)
> > > +        goto out;
> > > +
> >
> > I would rather this check be done in device_model_postconfig_vnc.
> >
> > Does the following work for you?
>
> I like your version, but it doesn't work:
> libxl: debug: libxl_qmp.c:1883:libxl__ev_qmp_send: Domain 1: ev
> 0x55aa58417d88, cmd 'query-vnc'
> libxl: error: libxl_qmp.c:1836:qmp_ev_parse_error_messages: Domain
> 1:The command query-vnc has not been found
> libxl: error: libxl_dm.c:3321:device_model_postconfig_done: Domain
> 1:Post DM startup configs failed, rc=-29
>
> When QEMU has vnc disabled, it doesn't recognize query-vnc.  I looked
> at modifying qemu to support query-vnc even with --disable-vnc, but it
> was messy to untangle the QMP definitions.  Since we are telling libxl
> not to use VNC, it makes sense not to query about it.

Oh, I should add that QEMU needs a small patch to allow -vnc none in
ui/vnc-stub.c when vnc is disabled.  This is because libxl always
passes -vnc none to ensure a default vnc is not created.

Regards,
Jason

>
> > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> > index a944181781bb..c5db755a65d7 100644
> > --- a/tools/libs/light/libxl_dm.c
> > +++ b/tools/libs/light/libxl_dm.c
> > @@ -3222,6 +3222,8 @@ static void device_model_postconfig_vnc(libxl__egc 
> > *egc,
> >
> >      if (rc) goto out;
> >
> > +    if (!vnc) goto out;
> > +
> >      /*
> >       * query-vnc response:
> >       * { 'enabled': 'bool', '*host': 'str', '*service': 'str' }
> > @@ -3255,7 +3257,8 @@ static void device_model_postconfig_vnc(libxl__egc 
> > *egc,
> >          if (rc) goto out;
> >      }
> >
> > -    if (vnc && vnc->passwd && vnc->passwd[0]) {
> > +    assert(vnc);
> > +    if (vnc->passwd && vnc->passwd[0]) {
> >          qmp->callback = device_model_postconfig_vnc_passwd;
> >          libxl__qmp_param_add_string(gc, &args, "password", vnc->passwd);
> >          rc = libxl__ev_qmp_send(egc, qmp, "change-vnc-password", args);
> >